faf8fc5487d409eeff7b7b260eabb6929a7b7a5f lint: Call lint_commit_msg from test_runner (MarcoFalke)
fa99728b0c8b3cac7056fa554fab7a8a4624a2de lint: Move commit range printing to test_runner (MarcoFalke)
fa673cf3449f4e71501814bf99c2e2bbb49b8fcb lint: Call lint_scripted_diff from test_runner (MarcoFalke)
Pull request description:
The lint `commit-script-check.sh` can not be called from the test_runner at all and must be called manually. Also, some checks require `COMMIT_RANGE` to be set.
Fix all issues by moving two lint checks into the test_runner. Also, the proper commit range is passed to the checks by the test_runner, so that the user no longer has to do it.
ACKs for top commit:
kevkevinpal:
reACK [faf8fc5](faf8fc5487)
willcl-ark:
tACK faf8fc5487d409eeff7b7b260eabb6929a7b7a5f
Tree-SHA512: 78018adc618d997508c226c9eee0a4fada3899cdfd91587132ab1c0389aea69127bafc3a900e90e30aca2c6bae9dcd6e6188ef287e91413bc63ee66fb078b1af
3e97ff9c5eaa3160426ba112930b047404c54c9e gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs (Ava Chow)
Pull request description:
SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot inputs, and SIGHASH_ALL for all other input types. This avoids adding an unnecessary byte to the end of all Taproot signatures added to PSBTs signed in the GUI.
See also bitcoin/bitcoin#22514
ACKs for top commit:
Sjors:
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e
pablomartin4btc:
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e
hebasto:
ACK 3e97ff9c5eaa3160426ba112930b047404c54c9e, I have reviewed the code and it looks OK.
Tree-SHA512: f96f26b3a6959865cf23039afb5ffb7e454fb52ee39c510583851caf00a8a383cde69bc7e90db536addbdd498a02f4b001cbaf509d6d53c5f8601b3933786f6c
9d2d9f7ce29636f08322df70cf6abec8e0ca3727 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr)
595edee169045b6735b76ff9721677f0e43f13e5 test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia)
d73ae603d44f93e4d6c5116f235dd11a0bdbf89c rpc: Improve importdescriptor RPC error messages (Fabian Jahr)
27f99b6d63b7ca2d4fcb9db3e88ed66c024c59d5 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr)
42d5d5336319aaf0f07345037db78239d9e012fc interfaces: Add helper function for wallet on pruning (Fabian Jahr)
Pull request description:
A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915ee6b) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet.
The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning.
The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change.
This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active.
ACKs for top commit:
achow101:
ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
mzumsande:
Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
furszy:
Code review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365
0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d kernel: Move block tree db open to BlockManager constructor (TheCharlatan)
7fbb1bc44b1461f008284533f1667677e729f0c0 kernel: Move block tree db open to block manager (TheCharlatan)
57ba59c0cdf20de322afabe4a132ad17e483ce77 refactor: Remove redundant reindex check (TheCharlatan)
Pull request description:
Before this change the block tree db was needlessly re-opened during startup when loading a completed snapshot. Improve this by letting the block manager open it on construction. This also simplifies the test code a bit.
The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
ACKs for top commit:
maflcko:
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d 🏪
achow101:
ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
mzumsande:
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
Tree-SHA512: fe3d557a725367e549e6a0659f64259cfef6aaa565ec867d9a177be0143ff18a2c4a20dd57e35e15f97cf870df476d88c05b03b6a7d9e8d51c568d9eda8947ef
93747d934b8a5a1732732a958c0d7e2f5dd0b8c2 test: Added coverage to the waitfornewblock rpc (kevkevinpal)
Pull request description:
Added a test for the Negative timeout error if the rpc is given a negative value for its timeout arg
This adds coverage to the `waitfornewblock` rpc
you can check to see there is no coverage for this error by doing
`grep -nri "Negative timeout" ./test/`
and nothing shows up, you can also see by manually checking where we call `waitfornewblock` in the functional tests
ACKs for top commit:
Sjors:
tACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2
achow101:
ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2
brunoerg:
code review ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2
tdb3:
ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2
Tree-SHA512: 45cf34312412d3691a39f003bcd54791ea16542aa3f5a2674d7499c9cc4039550b2cbd32cc3d4c5fe100d65cb05690594b10a0c42dfab63bcca3dac121bb195b
e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9 doc: release notes (Sjors Provoost)
0082f6acc1dc6c99007e232fc8f849ed8147fc9f rpc: have mintime account for timewarp rule (Sjors Provoost)
79d45b10f1b354b53fe7244b0c4d5e603beec700 rpc: clarify BIP94 behavior for curtime (Sjors Provoost)
071354813783768e3dec3b209b539e3d0fd7a1a0 refactor: add GetMinimumTime() helper (Sjors Provoost)
Pull request description:
#30681 fixed the `curtime` field of `getblocktemplate` to take the timewarp rule into account. However I forgot to do the same for the `mintime` field, which was hardcoded to use `pindexPrev->GetMedianTimePast()+1`.
This PR adds a helper `GetMinimumTime()` and uses it for the `mintime` field.
#31376 changed the `curtime` field to always account for the timewarp rule. This PR maintains that behavior.
Note that `mintime` now always applies BIP94, including on mainnet. This makes future softfork activation safer.
It could be backported to v28.
ACKs for top commit:
fjahr:
tACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9
achow101:
ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9
darosior:
utACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9 on the code changes
tdb3:
brief code review re ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9
TheCharlatan:
ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9
Tree-SHA512: 0e322d8cc3b8ff770849bce211edcb5b6f55d04e5e0dee0657805049663d758f27423b047ee6363bd8f6c6fead13f974760f48b3321ea86f514f446e1b23231c
This prevents the generation of these headers from also depending on the
dependencies of the libs/binaries which consume them.
Specifically, this prevents generated test headers (such as
test/data/base58_encode_decode.json.h) from depending on the
dependencies of test_bitcoin (libcrc32c.a libcrc32c_sse42.a libleveldb.a)
Note that this is currently only relevant for Ninja.
For more detail, see:
https://cmake.org/cmake/help/latest/command/add_custom_command.html
8888ee4403fa62972c49e18752695d15fd32c0b0 ci: Allow build dir on CI host (MarcoFalke)
Pull request description:
This is required to pass cross builds on to a different machine after the build.
See for example https://github.com/bitcoin/bitcoin/pull/31176, but this pull will also allow someone to implement it outside this repo.
ACKs for top commit:
davidgumberg:
lgtm ACK 8888ee4403
hebasto:
re-ACK 8888ee4403fa62972c49e18752695d15fd32c0b0.
Tree-SHA512: a1e2c32bc1b95efbd0b48287ac5b49e0e1bacbf5a5800845be5352bbdd3e17fa478e90348b2e94e95cf3ae863cdf75ab444089376588f6f8eec438f73a4b5b97
152a2dcdefa6ec744db5b106d5c0a8c5b8aca416 test: fix intermittent timeout in p2p_1p1c_network.py (Martin Zumsande)
Pull request description:
The timeout is due to outstanding txrequests with python peers, which have the same timeout (`60s`) as the mempool sync timeout.
I explained this in more detail in https://github.com/bitcoin/bitcoin/issues/31721#issuecomment-2620169640 and also mentioned there how to reproduce it.
Fix this by disconnecting the python peers after they send their txns, they aren't needed after this point anyway because the main goal of the test is the sync between the 4 full nodes.
Fixes#31721
ACKs for top commit:
achow101:
ACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416
instagibbs:
reACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416
marcofleon:
ACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416
glozow:
reACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416
Tree-SHA512: 908c58933d8e9fcca91425fce1b7c9c7cb7121a6d26840630e03a442356ad2a327d1e087df72a19caa97024ea827593e10f2ff93838f88939458e73df9857df0
Deduplicate the logic of adding the parents as announcements to
txrequest. The function can return a bool (indicating whether we're
attempting orphan resolution) instead of the delay.
e87429a2d0f23eb59526d335844fa5ff5b50b21f ci: optionally use local docker build cache (0xb10c)
Pull request description:
By setting `DANGER_DOCKER_BUILD_CACHE_HOST_DIR`, the task-specific docker images built during the CI run can be cached. This allows, for example, ephemeral CI runners to reuse the docker images (or layers of it) from earlier runs, by persisting the image cache before the ephemeral CI runner is shut down. The cache keyed by `CONTAINER_NAME`.
As `--cache-to` doesn't remove old cache files, the existing cache is removed after a successful `docker build` and the newly cached image is moved to it's location to avoid the cache from growing indefinitely with old, unused layers.
When `--cache-from` doesn't find the directory, the cached version is a cache-miss, or the cache can't be imported for whatever other reason, it warns and `docker build` continues by building the docker image.
This feature is opt-in. The documentation for the docker build cache of `type=local` can be found on https://docs.docker.com/build/cache/backends/local/
This replaces https://github.com/bitcoin/bitcoin/pull/31377 - some of the discussion there might provide more context.
ACKs for top commit:
maflcko:
I haven't tested this, and it looks harmless and is easy to revert, if needed. So lgtm ACK e87429a2d0f23eb59526d335844fa5ff5b50b21f
achow101:
ACK e87429a2d0f23eb59526d335844fa5ff5b50b21f
TheCharlatan:
tACK e87429a2d0f23eb59526d335844fa5ff5b50b21f
willcl-ark:
ACK e87429a2d0f23eb59526d335844fa5ff5b50b21f
Tree-SHA512: 0887c395dee2e2020394933246d4c1bfb6dde7165219cbe93eccfe01379e05c75dce8920b6edd7df07364c703fcee7be4fba8fa45fd0e0e89da9e24759f67a71
cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e RPC: improve SFFO arg parsing, error catching and coverage (furszy)
4f4cd353194310a8562da6aae27c9c8eda8a4ff0 rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing (furszy)
Pull request description:
Following changes were made:
1) Catch and signal error for duplicate string destinations.
2) Catch and signal error for invalid value type.
3) Catch and signal error for string destination not found in tx outputs.
4) Improved `InterpretSubtractFeeFromOutputInstructions()` code organization.
5) Added test coverage for all possible error failures.
Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
- PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration.
- PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params()
ACKs for top commit:
achow101:
ACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
murchandamus:
crACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
naiyoma:
TACK [cddcbaf81e)
ismaelsadeeq:
Code review and Tested ACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
Tree-SHA512: c9c15582b81101a93987458d155394ff2c9ca42864624c034ee808a31c3a7d7f55105dea98e86fce17d3c7b2c1a6b5b77942da66b287f8b8881a60cde78c1a3c
d45eb3964f693eddcf96f1e4083cf19d327be989 test: compare BDB dumps of test framework parser and wallet tool (Sebastian Falbesoner)
01ddd9f646a5329a92341bb216f3757fa97c0709 test: complete BDB parser (handle internal/overflow pages, support all page sizes) (Sebastian Falbesoner)
Pull request description:
This PR adds missing features to our test framework's BDB parser with the goal of hopefully being able to read all legacy wallets that are created with current and past versions of Bitcoin Core. This could be useful both for making review of https://github.com/bitcoin/bitcoin/pull/26606 easier and to also possibly improve our functional tests for the wallet BDB-ro parser by additionally validating it with an alternative implementation. The second commits introduces a test that create a legacy wallet with huge label strings (in order to create overflow pages, i.e. pages needed for key/value data than is larger than the page size) and compares the dump outputs of wallet tool and the extended test framework BDB parser.
It can be exercised via `$ ./test/functional/tool_wallet.py --legacy`. BDB support has to be compiled in (obviously).
For some manual tests regarding different page sizes, the following patch can be used:
```diff
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 38cca32f80..1bf39323d3 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -395,6 +395,7 @@ void BerkeleyDatabase::Open()
DB_BTREE, // Database type
nFlags, // Flags
0);
+ pdb_temp->set_pagesize(1<<9); /* valid BDB pagesizes are from 1<<9 (=512) to <<16 (=65536) */
if (ret != 0) {
throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile));
```
I verified that the newly introduced test passes with all valid page sizes between 512 and 65536.
ACKs for top commit:
achow101:
ACK d45eb3964f693eddcf96f1e4083cf19d327be989
furszy:
utACK d45eb3964f693eddcf96f1e4083cf19d327be989
brunoerg:
code review ACK d45eb3964f693eddcf96f1e4083cf19d327be989
Tree-SHA512: 9f8ac80452545f4fcd24a17ea6f9cf91b487cfb1fcb99a0ba9153fa4e3b239daa126454e26109fdcb72eb1c76a4ee3b46fd6af21dc318ab67bd12b3ebd26cfdd
551a09486c495e1a3cfc296eafdf95e914856bff net: Switch to DisconnectMsg in CConnman (Hodlinator)
bbac17608d1ad3f8af5b32efad5d573c70989361 net: Bring back log message when resetting socket (Hodlinator)
04b848e4827f502d0784c5975bc8e652fc459cc8 net: Specify context in disconnecting log message (Hodlinator)
0c4954ac7d9676774434e5779bb5fd88e789bbb6 net_processing: Add missing use of DisconnectMsg (Hodlinator)
Pull request description:
- Add missing calls to `DisconnectMsg()` - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
- Specify context when stopping nodes - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890780754
- Bring back log message when resetting socket in case new entrypoints are added - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890795074
- Use `DisconnectMsg()` in `CConnman` as well - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791797716
ACKs for top commit:
Sjors:
re-utACK 551a09486c495e1a3cfc296eafdf95e914856bff
l0rinc:
utACK 551a09486c495e1a3cfc296eafdf95e914856bff
davidgumberg:
Tested and Review ACK 551a09486c
achow101:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
danielabrozzoni:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
Tree-SHA512: 95ab8e7436e20ca3abc949ea09697facb6fbeb19981ddc7e0bf294e7ec914e72cbf836c21184a2a887f04cb264f26daf5b0cbcbebc9db633a7b1672b4e488063
p2p_orphan_handling checks whether a message has not been requested slightly
too soon, making the check always succeed. This passes unnoticed since the
expected result is for the message to not have been received, but it will make
the test not catch a relevant change that should make it fail
p2p_ibd_txrelay expects no GETDATA to have been received by a peer after
announcing a transaction. The reason is that the node is doing IBD, so
transaction requests are not replied to. However, the way this is checked
is wrong, and the check will pass even if the node **was not** in IBD.
This is due to the mocktime not being properly initialized, so the check
is always performed earlier than it should, making it impossible for the
request to be there
The timeout is due to outstanding txrequests with
python peers. Fix this by disconnecting these peers
after they send their txns, they aren't needed after
this point anyway.
Previously in getblocktemplate only curtime took the timewarp rule into account.
Mining pool software could use either, though in general it should use curtime.
Before bip94 there was an assumption that the minimum permitted
timestamp is GetMedianTimePast() + 1.
This commit splits a helper function out of UpdateTime() to
obtain the minimum time in a way that takes the
timewarp attack rule into account.
fa8ade300f421dcc3b0cd956ab03a50a5ae80646 refactor: Avoid GCC false positive error (MarcoFalke)
fa40807fa830ee9724b5cfeef263263aaa6ce5d9 ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions (MarcoFalke)
Pull request description:
It is possible that someone accidentally removes the workaround in fa9e0489f57968945d54ef56b275f51540f3e5e4, or more likely that someone accidentally adds new code without the workaround.
Avoid this by adding a temporary CI check.
This can be tested by reverting the workaround and observing a failure.
ACKs for top commit:
hebasto:
ACK fa8ade300f421dcc3b0cd956ab03a50a5ae80646, I've tested locally on Ubuntu 24.04.
Tree-SHA512: 7ee1538fd5304a5ab91ac8c7619a573548d7e0345592a1e9d38b3b73729e09e7c77a9ee703d64cf02a8218de3148376d7836e294abb939aa7533034ba36dfb6c
faf2f2c654d9aa18b2f49a157956f9ab0fce302a test: Avoid redundant stop and error spam on shutdown (MarcoFalke)
fae3bf6b870eb0f9cddd1adac82ba72890806ae3 test: Avoid redundant stop and error spam on startup failure (MarcoFalke)
fa0dc09b9002f0bcae63af6af8d37fb3e0040ef4 test: Remove --noshutdown flag (MarcoFalke)
fad441fba07877ea78ed6020fde11828307273a6 test: Treat leftover process as error (MarcoFalke)
Pull request description:
The `--noshutdown` flag is brittle, confusing, and redundant:
* Someone wanting to inspect the state after a test failure will likely also want to debug the state on the python side, so the option is redundant with `--pdbonfailure`. If there was a use case to replicate `--pdbonfailure` without starting pdb, a dedicated flag could be added for that use case.
* It is brittle to use the flag for a passing test, because it will disable checks in the test. For example, on shutdown LSan will perform a leak check, and the test framework will check that the node did not crash, and it will check that the node did not print errors to stderr.
Fix all issues by removing it.
Also, tidy up startup error messages to be less confusing as a result.
ACKs for top commit:
hodlinator:
re-ACK faf2f2c654d9aa18b2f49a157956f9ab0fce302a
pablomartin4btc:
re tACK faf2f2c654d9aa18b2f49a157956f9ab0fce302a
Tree-SHA512: 46d7ae59c7be88b93f1f9e0b6be21af0fc101e646512e2c5e725682cb18bfec8aa010e0ebe89ce9ffe239e5caac0da5f81cc97b79e738d26ca5fa31930e8e4e3
f5883286e32b625aab3dd80c74d6adb4f37f0a80 Add a fuzz test for Num3072 multiplication and inversion (Pieter Wuille)
a26ce628942243fc9848a63bfdfa5e61f5e936f3 Safegcd based modular inverse for Num3072 (Pieter Wuille)
91ce8cef2d8955d980ab7e89fbf74e8b29adf178 Add benchmark for MuHash finalization (Pieter Wuille)
Pull request description:
This implements a safegcd-based modular inverse for MuHash3072. It is a fairly straightforward translation of [the libsecp256k1 implementation](https://github.com/bitcoin-core/secp256k1/pull/831), with the following changes:
* Generic for 32-bit and 64-bit
* Specialized for the specific MuHash3072 modulus (2^3072 - 1103717).
* A bit more C++ish
* Far fewer sanity checks
A benchmark is also included for MuHash3072::Finalize. The new implementation is around 100x faster on x86_64 for me (from 5.8 ms to 57 μs); for 32-bit code the factor is likely even larger.
For more information:
* [Original paper](https://gcd.cr.yp.to/papers.html) by Daniel J. Bernstein and Bo-Yin Yang
* [Implementation](https://github.com/bitcoin-core/secp256k1/pull/767) for libsecp256k1 by Peter Dettman; and the [final](https://github.com/bitcoin-core/secp256k1/pull/831) version
* [Explanation](https://github.com/bitcoin-core/secp256k1/blob/master/doc/safegcd_implementation.md) of the algorithm using Python snippets
* [Analysis](https://github.com/sipa/safegcd-bounds) of the maximum number of iterations the algorithm needs
* [Formal proof in Coq](https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348) by Russell O'Connor (for the 256-bit version of the algorithm; here we use a 3072-bit one).
ACKs for top commit:
achow101:
ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
TheCharlatan:
Re-ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
dergoegge:
tACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
Tree-SHA512: 275872c61d30817a82901dee93fc7153afca55c32b72a95b8768f3fd464da1b09b36f952f30e70225e766b580751cfb9b874b2feaeb73ffaa6943c8062aee19a
the cmake build steps suggest a build/ directory, which breaks these
scripts. Additionally, in-tree builds are no longer allowed, so it makes
sense to update the code and the README accordingly.
