83df64d749 log: Stats when fulfilling GETBLOCKTXN (David Gumberg)
3733ed2dae log: Size of missing tx'es when reconstructing compact block (David Gumberg)
36bcee05dc log: Log start of compact block initialization. (David Gumberg)
Pull request description:
This PR adds some additional logging to help measure performance of compact block reconstruction.
1. Adds a message to the beginning of `PartiallyDownloadedBlock::InitData()` so that that the logs indicate the amount of time it takes to populate a compact block from mempool transactions.
2. Logs the size of the transactions which a node did not have in its mempool and was forced to request.
3. Logs the size and number of transactions that a node sends to it's peer in a `BLOCKTXN` to fulfill a compact block `GETBLOCKTXN` request.
Relevant to this discussion on delving bitcoin: https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052
ACKs for top commit:
instagibbs:
reACK 83df64d749
w0xlt:
reACK 83df64d749
1440000bytes:
ACK 83df64d749
Tree-SHA512: 92c3c7d55005dd47dad90ddb54e4127482260cea5390f7696e8b3b9defb337f5fb09166af6b12eb2ab8151d04dae08b0a570e3509a86509b0ab3151d84387e06
84aa484d45 test: fix transaction_graph_test reorg test (Greg Sanders)
eaf44f3767 test: check chainlimits respects on reorg (Greg Sanders)
47894367b5 functional test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage (Greg Sanders)
Pull request description:
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
Another test added is making sure chainlimits are being respected on reorg, and the expected transactions pruned.
Lastly, fix the existing test case which was using a deficient test via directly inducing reorgs with `invalidateblock`
ACKs for top commit:
maflcko:
re-ACK 84aa484d45🚋
TheCharlatan:
ACK 84aa484d45
Tree-SHA512: f5cdb9647fadc8eb30352ce38de44064103825e5358787dfccd6416fa8faf6ceea42552fe2250b37d56271a6c3898b3912e1c028652da122f5c99304aafddb64
dbb2d4c3d5 windows: Add application manifest to `bitcoin.exe` (Hennadii Stepanov)
df82c2dc17 windows: Add resource file for `bitcoin.exe` (Hennadii Stepanov)
Pull request description:
This PR is a follow up to https://github.com/bitcoin/bitcoin/pull/31375, which:
1. Adds a resource file for `bitcoin.exe` for consistency with other Windows executables.
2. Adds an application manifest to `bitcoin.exe`, which has been required for release binaries since https://github.com/bitcoin/bitcoin/pull/32396.
ACKs for top commit:
davidgumberg:
ACK dbb2d4c3d5
hodlinator:
ACK dbb2d4c3d5
Tree-SHA512: 853c9e578bfd74bfd2e1f0fa39f978638723c8e061456caa165fca6f10497517f9503ae12dfb88e7229a02de593ccf22126f3362ca0d75c74becbb727e80c9ad
4df4df45d7 test: fix sync function in rpc_psbt.py (Martin Zumsande)
Pull request description:
Even though the block is created on `node2`, the sync is only between `node1` and `node0`. Accordingly the test fails if I put a sleep in `msg_type == NetMsgType::HEADERS` processing: In this case, `node1` and `node0` do not hear about the new block, the sync still passes because they are in sync with each other, and later on in the `test_input_confs_control` subtest, `node1` would generate a forked block instead of building on the previous one, leading to test failure.
Haven't seen this in the CI, but I ran into it on an experimental branch.
ACKs for top commit:
maflcko:
lgtm ACK 4df4df45d7
achow101:
ACK 4df4df45d7
Tree-SHA512: 1211ba0ad263ebcd0aa6ef7c856dec7ec6ca6010e1df705e7243f6c9d950ccca6df1275c36a73a83034f49ea8401e8f9800c05cdb74c39e860e7ebcaf2ce6ada
fab1e02086 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdf test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086
pinheadmz:
ACK fab1e02086
sipa:
utACK fab1e02086
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
3e6ac5bf77 refactor: validation: mark CheckBlockIndex as const (stickies-v)
61a51eccbb validation: don't use GetAll() in CheckBlockIndex() (stickies-v)
d05481df64 refactor: validation: mark SnapshotBase as const (stickies-v)
Pull request description:
While reviewing another PR, I [noticed](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235) that `ChainstateManager::CheckBlockIndex()` is not a `const` method. To try and assert that this method was not causing any side-effects, I modified the method to make it `const`. It did not surface any errors, but I think it would be good to merge this change regardless, even if `CheckBlockIndex` is only used in regtest.
This PR removes `CheckBlockIndex()`'s calls to non-const `ChainstateManager` methods by marking `SnapshotBase` `const` and ~inlining the `GetAll()` calls (thereby also performing consistency checks on invalid or fully validated `m_disabled==true` chainstates, as slight behaviour change), and finally marks `CheckBlockIndex()` as `const`.
ACKs for top commit:
achow101:
ACK 3e6ac5bf77
mzumsande:
Code Review ACK 3e6ac5bf77
TheCharlatan:
ACK 3e6ac5bf77
Tree-SHA512: 3d3cd351f5af1fab9a9498218ec62dba6e397fc7b5f4868ae0a77dc2b7c813d12c4f53f253f209101a3f6523695014e20c82dfac27cf0035611d5dd29feb80b5
The current test directly uses invalidatblock to trigger
mempool re-entry of transactions. Unfortunately, the
behavior doesn't match what a real reorg would look like. As
a result you get surprising behavior such as the mempool
descendant chain limits being exceeded, or if a fork is
greater than 10 blocks deep, evicted block transactions stop
being submitted back into in the mempool.
Fix this by preparing an empty fork chain, and then
continuing with the logic, finally submitting the fork chain
once the rest of the test is prepared. This triggers a more
typical codepath.
Also, extend the descendant limit to 100, like ancestor
limit.
09ee8b7f27 node: avoid recomputing block hash in `ReadBlock` (Lőrinc)
2bf173210f test: exercise `ReadBlock` hash‑mismatch path (Lőrinc)
Pull request description:
Eliminate one block header hash calculation per block-read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.
ACKs for top commit:
maflcko:
lgtm ACK 09ee8b7f27
achow101:
ACK 09ee8b7f27
jonatack:
ACK 09ee8b7f27
pinheadmz:
ACK 09ee8b7f27
Tree-SHA512: 43fe51b478ea574b6d4c952684b13ca54fb8cbd67c3b6c136f460122d9ee953cc70b88778537117eecea71ccb8d88311faeac21b866e11d117f1145973204ed4
a5ac43d98d doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c0 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c9 build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c0006 ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d
achow101:
ACK a5ac43d98d
vasild:
ACK a5ac43d98d
theStack:
ACK a5ac43d98d
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d
hodlinator:
ACK a5ac43d98d
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
f66b14d2ec test: fix pushdata scripts (Greg Sanders)
Pull request description:
The original scripts were done incorrectly,
so they are changed to represent two
different 2-byte pushes.
Fixes https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2034051063
ACKs for top commit:
ajtowns:
ACK f66b14d2ec
TheCharlatan:
Re-ACK f66b14d2ec
Tree-SHA512: 0956124ee0d2e8b6a594f9feeb47c1f598c68e24d277e874f81a093268113e9da2c75a02863dbaab68b962063f7d910bfd10abe3ad33ec182bc21d72908f06e6
Eliminate one SHA‑256 double‑hash computation of the header per block read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
e5cbea416b rpc: doc: remove redundant "descriptors" parameter in `createwallet` examples (Sebastian Falbesoner)
7a05f941bb rpc: doc: drop descriptor wallet mentions in fast wallet rescan related RPCs (Sebastian Falbesoner)
db465a50e2 wallet, rpc: remove obsolete "keypoololdest" result field/code (Sebastian Falbesoner)
Pull request description:
This PR contains a few smaller wallet RPC cleanups based on that we only ever operate on descriptor wallets now:
* remove the now obsolete "keypoololdest" field from the `getwalletinfo` RPC and the corresponding CWallet/ScriptPubKeyMan methods
* in RPCs where potential fast wallet rescan is documented, remove the "descriptor wallet" mentions (back then introduced in commit ca48a4694f, PR #25957)
* for the `createwallet` RPC examples, remove the "descriptors" parameters that always have to be true now (proposed in https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2042020967; corresponds to 86de8c1668, PR #32544 which did the same for functional tests)
ACKs for top commit:
achow101:
ACK e5cbea416b
1440000bytes:
ACK e5cbea416b
rkrux:
ACK e5cbea416b
Tree-SHA512: d785f621af3f3987b258e5d7fb8309344fb13c2cf41855f8adf99ff89f581142db36e3ba59919d6abf82662caa3f7e4a2bd38eba1be9e23665e6a4a23ee52acd
This is the RPC example counterpart to commit
86de8c1668 (PR #32544).
Since the recent legacy wallet removal this parameter *must* be
true, so providing it in the examples doesn't contain valuable
information anymore and it seems best to remove them.
Now that we only ever operate on descriptor wallets, mentioning
that a faster rescan is only available for them is redundant and
can be removed.
These texts were originally introduced in commit
ca48a4694f (PR #25957).
This `getwalletinfo()` result field was only ever returned for
legacy wallets and is hence not relevant anymore, so we can
delete it and the corresponding CWallet/ScriptPubKeyMan code
behind it.
fa079538e3 ci: Downgrade DEBUG=1 to -D_GLIBCXX_ASSERTIONS in centos task (MarcoFalke)
Pull request description:
to work around https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2900903169closes#32524
ACKs for top commit:
laanwj:
ACK fa079538e3
fanquake:
ACK fa079538e3 - we can followup
Tree-SHA512: 2d8b914c7390bbf22d8b2eb906bd2a363f92e1954646677a010b15721fca0887d5987a0af932fd0013f5c1b35c0a80c67579004a0cf635916954331c80c7bef0
fd290730f5 validation: clean up and clarify CheckInputScripts logic (Cory Fields)
1a37507895 validation: use a lock for CCheckQueueControl (Cory Fields)
c3b0e6c7f4 validation: make CCheckQueueControl's CCheckQueue non-optional (Cory Fields)
4c8c90b556 validation: only create a CCheckQueueControl if it's actually going to be used (Cory Fields)
11fed833b3 threading: add LOCK_ARGS macro (Cory Fields)
Pull request description:
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.
This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally. In the case of validation, it is wrapped in a `std::optional`.
It also adds an `LOCK_ARGS` macro for `UniqueLock` initialization which may be helpful elsewhere.
ACKs for top commit:
fjahr:
re-ACK fd290730f5
ryanofsky:
Code review ACK fd290730f5, just removing assert since last review. Thanks for considering all the comments and feedback!
TheCharlatan:
Re-ACK fd290730f5
Tree-SHA512: 54b9db604ee1bda7d11bce1653a88d3dcbc4f525eed6a85abdd4d6409138674af4bb8b00afa4e0d3d29dadd045a3a39de253a45f0ef9c05f56cba1aac5b59303
7bc64a8859 test: properly check for per-tx sigops limit (Sebastian Falbesoner)
Pull request description:
Currently the per-tx sigops limit standardness check (bounded by `MAX_STANDARD_TX_SIGOPS_COST`, throwing "bad-txns-too-many-sigops" if exceeded):
3f83c744ac/src/validation.cpp (L925-L927)
is only indirectly tested with the much higher per-block consensus limit (`MAX_BLOCK_SIGOPS_COST`):
3f83c744ac/test/functional/data/invalid_txs.py (L236-L242)
I.e. an increase in the per-tx limit up to the per-block one would still pass all of our tests. Refine that by splitting up the invalid tx template `TooManySigops` in a per-block and a per-tx template.
The involved functional tests taking use of these templates are `feature_block.py` and `p2p_invalid_txs.py`. Can be tested by applying e.g.
```diff
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 2151ec13dd..e5766d2a55 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -37,7 +37,7 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65};
/** Maximum number of signature check operations in an IsStandard() P2SH script */
static constexpr unsigned int MAX_P2SH_SIGOPS{15};
/** The maximum number of sigops we're willing to relay/mine in a single tx */
-static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5};
+static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5 + 4};
/** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or replacement **/
static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
/** Default for -bytespersigop */
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
```
where the tests succeed on master, but fail on this PR.
(Found by diving deeper into the jungle of current sig-ops limit, as preparation for reviewing the [BIP 54](https://github.com/bitcoin/bips/blob/master/bip-0054.md) draft and related preparatory PRs like #32521).
ACKs for top commit:
fjahr:
tACK 7bc64a8859
tapcrafter:
tACK 7bc64a8859
darosior:
ACK 7bc64a8859
instagibbs:
crACK 7bc64a8859
Tree-SHA512: 1365409349664a76a1d46b2fa358c0d0609fb17fffdd549423d22b61749481282c928be3c2fb428725735c82d319b4279f703bde01e94e4aec14bab206abb8cf
800b7cc42c cmake: Add missed `SSE41_CXXFLAGS` (Hennadii Stepanov)
028476e71f cmake: Remove `ENABLE_ARM_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
1e900528d2 cmake: Remove `ENABLE_X86_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
8689628e2e cmake: Remove `ENABLE_AVX2` from `bitcoin-build-config.h` (Hennadii Stepanov)
a8e2342dca cmake: Remove `ENABLE_SSE41` from `bitcoin-build-config.h` (Hennadii Stepanov)
Pull request description:
`ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` are already conditionally defined for the [`bitcoin_crypto`](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/CMakeLists.txt) target, and they are not used by any other targets. Defining them globally in `bitcoin-build-config.h` is therefore redundant.
Additionally, the previously missing `SSE41_CXXFLAGS` variable has been [added](https://github.com/bitcoin/bitcoin/pull/32550#issuecomment-2890918551).
ACKs for top commit:
fanquake:
ACK 800b7cc42c
Tree-SHA512: da792a0b780c67b432b09c9288ca98d62545315c721fed13510d1c11f8bb0cddd9a4ed7a009b4d052471dda19d0641bbc1eae4805fc306d23bf9b4ef510089c8
785e1407b0 wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0
ryanofsky:
Code review ACK 785e1407b0
furszy:
Code review ACK 785e1407b0
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
fab97f583f ci: Avoid && dropping errors (MarcoFalke)
Pull request description:
In bash, `&&` will ignore errexit. This can lead to silently ignoring errors. Compare the output of:
```
$ bash -c 'set -xe; false && false ; true; echo $?'
+ false
+ true
+ echo 0
0
```
In theory this could be fixed by using a subshell:
```
$ bash -c 'set -xe; ( false && false ) ; true; echo $?'
+ false
```
However, it is easier to just remove the `&&`.
This was introduced in commit faa807bdf8
ACKs for top commit:
janb84:
Code review ACK fab97f583f
hebasto:
ACK fab97f583f.
laanwj:
ACK fab97f583f
Tree-SHA512: 9d034829e03ef3aefdaad82c3cab59bf3fe18529762271c1ad3c838357e337e94bd403b77e30c0cf69715254b65addff6d12f2fb497d7a0e2cdcbcbf78858d47
e8661aac75 wallet: drop watch-only things from interface (Sjors Provoost)
e99188e7da qt: drop unused watch-only functionality (Sjors Provoost)
Pull request description:
The watch-only functionality in the GUI was only used for legacy wallets. Watch-only descriptor wallets do not use this.
The only visible changes of this PR should be:
- dropped "Spendable:" label from the overview tab
- column width cache is reset
This PR also removes some unused variables from the interface.
ACKs for top commit:
davidgumberg:
Review ACK e8661aac75.
hebasto:
ACK e8661aac75, I have reviewed the code and it looks OK. The `src/qt/forms/overviewpage.ui` form was reviewed in Qt Designer.
Tree-SHA512: d7edb0f167e0b934075398a76eddca69890bb36848a918c932b1c2cea85ee87285e876cbfdf1f6dec7adf26b9f405fb558c70bec0c84585c0a9df33c2af78393
ee045b61ef rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c372 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06 psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337a wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49 psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a118256948 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4a wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923 psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd81532 tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d612 psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61ef
rkrux:
re-ACK ee045b61ef
fjahr:
Code review ACK ee045b61ef
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
97d383af6d Test updating non-ranged descriptor with [0,0] range succeeds (Novo)
2ae1788dd4 Skip range verification for non-ranged desc (Novo)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/31728
This PR updates the `DescriptorScriptPubKeyMan` to skip range checks for non-ranged descriptors, which previously caused errors when updating a non-ranged descriptor with the range [0,0]
#### Testing
A unit test was added to test the new behaviour
ACKs for top commit:
achow101:
ACK 97d383af6d
rkrux:
ACK 97d383a
Tree-SHA512: 6dbd058376d9e57d26477d9d6d89646e80a32e3ffcc9f4e30eeda273575d12583ce520cc0032cc67c12ea0b3ad344fbd3945d9fc5e389b6a6bce1ea7ad5d6e59
6f7052a7b9 threading: semaphore: move CountingSemaphoreGrant to its own header (Cory Fields)
fd15469892 threading: semaphore: remove temporary convenience types (Cory Fields)
1f89e2a49a scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones (Cory Fields)
f21365c4fc threading: replace CountingSemaphore with std::counting_semaphore (Cory Fields)
1acacfbad7 threading: make CountingSemaphore/CountingSemaphoreGrant template types (Cory Fields)
e6ce5f9e78 scripted-diff: rename CSemaphore and CSemaphoreGrant (Cory Fields)
793166d381 wallet: change the write semaphore to a BinarySemaphore (Cory Fields)
6790ad27f1 scripted-diff: rename CSemaphoreGrant and CSemaphore for net (Cory Fields)
d870bc9451 threading: add temporary semaphore aliases (Cory Fields)
7b816c4e00 threading: rename CSemaphore methods to match std::semaphore (Cory Fields)
Pull request description:
This is relatively simple, but done in a bunch of commits to enable scripted diffs.
I wanted to add a semaphore in a branch I've been working on, but it was unclear if I should use `std::counting_semaphore` or stick with our old `CSemaphore`. I couldn't decide, so I just decided to remove all doubt and get rid of ours :)
This replaces our old `CSemaphore` with `std::counting_semaphore` everywhere we used it. `CSemaphoreGrant` is still there as an RAII wrapper, but is now called `CountingSemaphoreGrant` and `BinarySemaphoreGrant` to match. Those have been moved out of `sync.h` to their own file.
ACKs for top commit:
purpleKarrot:
ACK 6f7052a7b9
achow101:
ACK 6f7052a7b9
TheCharlatan:
ACK 6f7052a7b9
hebasto:
ACK 6f7052a7b9, I have reviewed the code and it looks OK.
Tree-SHA512: 5975d13aa21739174e3a22c544620ae3f36345f172b51612346d3b7baf0a07c39ef6fd54f647c87878c21a67951b347a5d4a5f90e897f3f6c0db360a3779d0df
