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
Instead of allowing users to load a legacy wallet from the "Open Wallet"
menu, show the legacy wallet greyed out with a message that the wallet
needs to be migrated.
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
Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgrade
does not run and set the flag.
Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.
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
On Linux systems, `_POSIX_C_SOURCE` will default to `200809L` (since
glibc 2.10). There's currently no reason for us to undefine it, and then
set it to an earlier value. Also tested with musl libc.
I think if anything, the project should be settings macros like
`_POSIX_C_SOURCE`, globally.
Covers:
- http pipelining
- rpcservertimeout
Testing this requires adding an option to TestNode to force
the test framework to establish a new HTTP connection for
every RPC. Otherwise, attempting to reuse a persistent connection
would cause framework RPCs during startup and shutdown to fail.
- "chunked" Transfer-Encoding
This is already checked by test/lint/lint-files.py
There is no need to reword all linters into the dev notes.
Also, allow scripts in Rust (there are already some).
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
