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
135a0f0aa7 doc: Add missing top-level description to pruneblockchain RPC (nervana21)
Pull request description:
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.
This PR adds a top-level description, improving documentation consistency and alerting users to the potential impacts of using the command.
ACKs for top commit:
maflcko:
lgtm ACK 135a0f0aa7
yancyribbens:
cr ACK 135a0f0aa7
achow101:
ACK 135a0f0aa7
janb84:
re ACK [135a0f0](135a0f0aa7)
Tree-SHA512: e51475238e779555315668b7389ed312a5d2c4ad1c0b251f2314895ac473092fa458b6f931f70385e14047adb7e340e44fe2198643603da9e129f1c874578a28
CheckInputScripts behaves differently depending on whether or not it was called
with a vector for checks. Make this difference clear by calling it differently
depending on whether or not control exists. Though more verbose, it should be
more straightforward to understand what's happening this way.
Also remove parallel_script_checks, as `if(control)` is a better test.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
faf55fc80b doc: Remove ParseInt mentions in documentation (MarcoFalke)
3333282933 refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c36c bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face2519fa bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf0b9 bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a558 bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5fe3 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac037 rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb499d rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c691 test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123afa0e Reject + sign when checking -ipcfd (MarcoFalke)
fa479857ed Reject + sign in SplitHostPort (MarcoFalke)
fab4c2967d net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652e68 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c45577d cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa98041325 refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7fc2 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)
Pull request description:
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:
* Useless, because the `+` sign was already rejected.
* Erroneous and inconsistent, when third party parsers reject it. (C.f. https://github.com/bitcoin/bitcoin/pull/32365)
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.
Fix all issues by removing the legacy int parsing.
ACKs for top commit:
stickies-v:
re-ACK faf55fc80b
brunoerg:
code review ACK faf55fc80b
Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
4b2cd0b41f test: check that creating a wallet does not log version info (Ava Chow)
39a483c8e9 test: Check that the correct versions are logged on wallet load (Ava Chow)
359ecd3704 walletdb: Log the wallet version after it has been read from disk (Ava Chow)
Pull request description:
The wallet's version (in the minversion record) needs to be logged only after we have read it from disk. Otherwise, we always log the lowest version number of 10500 which is incorrect. Furthermore, it doesn't make sense to log the last client version number if the record didn't exist. This is a regression caused by #26021.
The wallet file version logging is moved inside of `LoadMinVersion` so that it is logged after the record is read. It will also log unconditionally if a version is read so that the version number is reported even when there is an error. The last client logging is split into its own log line that will only occur if a last client record is read. The only situation where we expect no version numbers to be logged is when a wallet is being created.
A test is added in the second commit to check that the version number is correctly logged on loading. This commit can be cherrypicked to master to verify that it fails there. The last commit adds an additional check that creating a new wallet does not log any version info at all.
ACKs for top commit:
laanwj:
Code review ACK 4b2cd0b41f
janb84:
ACK 4b2cd0b41f
furszy:
ACK 4b2cd0b41f
rkrux:
ACK 4b2cd0b41f
Tree-SHA512: b30c76f414d87be6c14b42d2d3c8794a91a7e8601501f4c24641d51ff2b5c5144776563baf41ca1c38415844740b760b19a3e5791f78013b39984dfedd3b1de7
The windows code adds an unnecessary extra space to the command line.
This can cause subtle issues, so avoid it.
Github-Pull: arun11299/cpp-subprocess#119
Rebased-From: 777cfa77d1f84bb08b3e445d5f7fc6c87282223b
fa58d6cdab ci: Move DEBUG=1 to centos task (MarcoFalke)
Pull request description:
The glibcxx debug mode has many bugs in prior gcc releases:
* https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890411766
* https://github.com/bitcoin/bitcoin/issues/31436#issuecomment-2530717875
* ...
Instead of working around all of them, just use the existing `ci_native_centos` task with gcc-14 to have it enabled. This also follows the logic of other sanitizers (tsan, asan, ubsan, msan, valgrind, ...) to generally prefer the latest version of the sanitizer for the latests features and bugfixes.
Fixes#32524.
Also, while touching the `ci_native_previous_releases`, increase g0 to g2, so that it is easier for developers to use gdb inside the CI without having to re-compile
ACKs for top commit:
hebasto:
ACK fa58d6cdab.
fanquake:
ACK fa58d6cdab
Tree-SHA512: 26f151887bc39e88353b4ba1f91e913b830d24eac258b7caa0027aa592595150d5e713ededa3ed15a1b84165a94b14b0bfe3eb2fb7314d261d972b63ce01af43
7193245cd6 doc: remove For ... comments (fanquake)
1b9cdc933f net: drop win32 ifdef (fanquake)
19ba499b1f init: cerrno is used on all platforms (fanquake)
Pull request description:
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs, if changed.
They are also wrong, i.e `DEFAULT_SCRIPTCHECK_THREADS` is not in
`validation.h`.
ACKs for top commit:
stickies-v:
re-ACK 7193245cd6
fjahr:
ACK 7193245cd6
willcl-ark:
reACK 7193245cd6
Tree-SHA512: 6b5f83cd1df699356e1cbb78949f8d456b13ce288f0064138118cfb45b4c77e2d1945babe91598dffe9823ab07dfae36f4c3b61c586cf98baf16890bdf322b08
fa9198af55 lint: Check for missing trailing newline (MarcoFalke)
fa2b2aa27c lint: Add archived notes to default excludes (MarcoFalke)
Pull request description:
A missing trailing newline is harmless, but a bit problematic:
* `git` shows a warning by default
* After another line is appended, the diff will be verbose and `git blame` will be wrong for the "untouched" line.
Fix the problems by just requiring what is already the default, see also 663a9cabf8/.editorconfig (L9) and 663a9cabf8/test/lint/test_runner/src/main.rs (L327)
ACKs for top commit:
l0rinc:
utACK fa9198af55
fanquake:
ACK fa9198af55
Tree-SHA512: d144eebdeee68fc3404aa4a66ecd5c130f907ed4b869bd300f6e9ed74d125561d1f4cdd6dd20d9e969471a7d007399f928f072d1c1f626275ca31f32bc23fdbc
In the dev notes, remove the whole section, because:
* ParseDouble was removed in commit
fa9d72a794
* The locale-dependent atoi is already checked by
test/lint/lint-locale-dependence.py
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
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.
