| Age | Commit message (Collapse) | Author |
|---|
|
|
|
params for mempool/contents
1ff5d61dfdaf8987e5619162662e4c760af76a43 doc: add mempool/contents rest verbose and mempool_sequence args (Andrew Toth)
52a31dccc92366efb36db3b94920bdf8b05b264c tests: mempool/contents verbose and mempool_sequence query params tests (Andrew Toth)
a518fff0f2e479064dd4cff6c29fb54c72c1407b rest: add verbose and mempool_sequence query params for mempool/contents (Andrew Toth)
Pull request description:
The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible.
It uses query parameters to be compatible with the efforts in https://github.com/bitcoin/bitcoin/issues/25752.
ACKs for top commit:
achow101:
ACK 1ff5d61dfdaf8987e5619162662e4c760af76a43
stickies-v:
re-ACK [1ff5d61](https://github.com/bitcoin/bitcoin/pull/26207/commits/1ff5d61dfdaf8987e5619162662e4c760af76a43)
pablomartin4btc:
tested ACK 1ff5d61dfdaf8987e5619162662e4c760af76a43.
Tree-SHA512: 1bf08a7ffde2e7db14dc746e421feedf17d84c4b3f1141e79e36feb6014811dfde80e1d8dbc476c15ff705de2d3c967b3081dcd80536d76b7edf888f1a92e9d1
|
|
05eeba2c5fb312e0e6a730b01eb7d1b422d75dbb [test] Add manual prune startup test case (dergoegge)
451741962885eaa4b55033d53af731e0ba22650f [util] Avoid integer overflow in CheckDiskSpace (dergoegge)
Pull request description:
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` ([here](https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/init.cpp#L1633-L1648)) because `nPruneTarget` is to the max `uint64_t` value.
```
node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
#0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
#1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
#2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43
#3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13
#4 0x7fcb7cbffd8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in
```
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file.
ACKs for top commit:
MarcoFalke:
ACK 05eeba2c5fb312e0e6a730b01eb7d1b422d75dbb 🥝
john-moffett:
ACK 05eeba2c5fb312e0e6a730b01eb7d1b422d75dbb
Tree-SHA512: 1d8e6bcb49818139f04b5ab2cbef7f9b422bf0c38a804cd532b6bd0ba4c4fd07f959ba977e59896343f213086c8ecc48180f50d006638dc84649c66ec379d58a
|
|
962a0930e699b74b3c4d019427df6e2b3af5c831 Improve address decoding errors (Aurèle Oulès)
Pull request description:
Attempt to fix #21741.
ACKs for top commit:
MarcoFalke:
lgtm ACK 962a0930e699b74b3c4d019427df6e2b3af5c831
davidgumberg:
ACK https://github.com/bitcoin/bitcoin/pull/26514/commits/962a0930e699b74b3c4d019427df6e2b3af5c831
1440000bytes:
utACK https://github.com/bitcoin/bitcoin/pull/26514/commits/962a0930e699b74b3c4d019427df6e2b3af5c831
Tree-SHA512: 6f216eeaeccf6bfdf0730d38835fdf26c935a5e1fc35006660393a9ad76bf38c85340f0f20e92f87840463d83d891d9714cfad313aab301a16bb8efa4490df06
|
|
fa27cf4cc7c24aa00a66dffabab849d4b3cb1c41 test: Default timeout factor to 4 under --valgrind (MarcoFalke)
Pull request description:
valgrind will incur a slowdown of at least 2, so increase the default timeout factor.
This should reduce the number of reported issues. See also https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455762739
ACKs for top commit:
fanquake:
ACK fa27cf4cc7c24aa00a66dffabab849d4b3cb1c41 - I still see at least one actual issue when running the functional tests under `--valgrind` (outside the CI system), but will follow up separately with that. Increasing the timeout here seems fine.
Tree-SHA512: 4467559a3bfd98f5735f300f6ed54c68f951191d65a2b1294d71d72cc5d0864964de562d5dfa0a4855fc541ccb269a538b7aeb3d408d2d012a5369513397c395
|
|
|
|
(`-bytespersigop` setting)
89cd20cbedbba344bab92dd1d71dac9c84320a70 test: add coverage for sigop limit policy (`-bytespersigop` setting) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `-bytespersigop` option, which determines how pre-taproot signature operations (OP_CHECKSIG{VERIFY}, OP_CHECKMULTIGSIG{VERIFY}) affect fee handling calculations. The setting was introduced in PR #7081 for mitigating the [sigop spam attack](https://bitcointalk.org/index.php?topic=1166928.0); the initial implementation rejected txs exceeding the limit, but was changed in #8365 later to account for higher sizes in the mempool (i.e. exceeding the sigop limit is possible, but has to be compensated by higher fees).
For each combination of `-bytespersigop` setting and sigops count, the test first creates a P2WSH spending transaction with a witness script that puts sigops in a non-executing branch (OP_FALSE OP_IF OP_CHECKMULTISIG ... OP_CHECKSIG ... OP_ENDIF). This tx is then bumped up to reach exactly the _sig-op limit equivalent vsize_ by padding its datacarrier output. Based on that, increasing the tx's vsize should still reflect a vsize increase in the mempool, while a decrease of the tx's vsize should lead to the mempool treating the tx's vsize to be the _sig-op limit equivalent vsize_, since the limit was exceeded.
I assume that this parameter is almost never set explicitly by users (also it is not relevant for taproot spends), but it doesn't hurt to have a test for it. See also https://bitcoin.stackexchange.com/a/87958 for another explanation.
ACKs for top commit:
glozow:
light review ACK 89cd20cbed
MarcoFalke:
nice ACK 89cd20cbedbba344bab92dd1d71dac9c84320a70 📁
Tree-SHA512: 06998ce93bf9d5ce6143db2996a43f13990c415f97afe684227ad469349e73952bf4f6c871c1e6349e07606f4d45db64408848873a86a89481cdca5a134e5e60
|
|
faa671591f9c83ef0fb5afea151a1907c28f024b test: Use self.wait_until over wait_until_helper (MarcoFalke)
Pull request description:
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.
ACKs for top commit:
theStack:
Code-review ACK faa671591f9c83ef0fb5afea151a1907c28f024b
Tree-SHA512: 70705f309f83ffd6ea5d090218195d05b868624d909106863372f861138b5a70887070b25beb25044ae1b44250345e45c9cc11191ae7aeca2ad37801a0f62f61
|
|
fe329dc936d1e02da406345e4223e11d1fa6fb38 test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr)
cd761e6b2cfade038b8018886c334bf25a0bcbcf rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr)
Pull request description:
These are additions to `getblockfrompeer` that I already [suggested on the original PR](https://github.com/bitcoin/bitcoin/pull/20295#pullrequestreview-817157738).
The two commits do the following:
1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node.
2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely.
ACKs for top commit:
Sjors:
re-utACK fe329dc936d1e02da406345e4223e11d1fa6fb38
MarcoFalke:
review ACK fe329dc936d1e02da406345e4223e11d1fa6fb38 🍉
stratospher:
ACK fe329dc.
brunoerg:
re-ACK fe329dc936d1e02da406345e4223e11d1fa6fb38
Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac
|
|
random fee in longpoll
fa0abcdafeb74aa7a312095becd55b1cee48cd99 test: Flatten miniwallet array and remove random fee in longpoll (MarcoFalke)
Pull request description:
* Using a single MiniWallet is enough.
* A random fee isn't needed either.
ACKs for top commit:
theStack:
re-ACK fa0abcdafeb74aa7a312095becd55b1cee48cd99
Tree-SHA512: 77b99885b3f0d325d067838122114be57ec999ebc82912de6a22c33e2ba28a341c5e053c5bbc424b9922c2616562289a57c7156bd3b431d779182c2e472da59c
|
|
`listdescriptors` and `importdescriptors`
b082f28101773e0ef0281d97025e65d0364d6f29 rpc, wallet: use the same `next_index` in listdescriptors and importdescriptors (w0xlt)
Pull request description:
Currently `listdescriptors` RPC uses `next` key to represent `WalletDescriptor::next_index` while `importdescriptors` uses `next_index`. This creates two different descriptor formats.
This PR changes `listdescriptors` to use the same key as `importdescriptors`.
ACKs for top commit:
achow101:
ACK b082f28101773e0ef0281d97025e65d0364d6f29
aureleoules:
reACK b082f28101773e0ef0281d97025e65d0364d6f29
Tree-SHA512: c29ec59051878e614d749ed6dc85e5c14ad00db0e8fcbce3f5066d1aae85ef07ca70f02920299e48d191b7387024fe224b0054c4191a5951cb805106f7b8e37b
|
|
|
|
|
|
|
|
|
|
to finish - 2nd attempt
60978c8080ec13ff4571c8a89e742517b2aca692 test: Reduce extended timeout on abortnode test (Fabian Jahr)
660bdbf785a32024f0694915fa043968a0afb573 http: Release server before waiting for event base loop exit (João Barbosa)
8c6d007c80dc3fec5ce6c0196381444a5ed7e424 http: Track active requests and wait for last to finish (João Barbosa)
Pull request description:
This revives #19420. Since promag is not so active at the moment, I can support this to finally get it merged.
The PR is rebased and comments by jonatack have been addressed.
Once this is merged, I will also reopen #19434.
ACKs for top commit:
achow101:
ACK 60978c8080ec13ff4571c8a89e742517b2aca692
stickies-v:
re-ACK [60978c8](https://github.com/bitcoin/bitcoin/commit/60978c8080ec13ff4571c8a89e742517b2aca692)
hebasto:
ACK 60978c8080ec13ff4571c8a89e742517b2aca692
Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
|
|
files on startup
3141eab9c669488a2e7fef5f60d356ac92294922 test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909e561e47d75cb3a892657662a139f6532c test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dda4a123515d0fa2a545ee21d7c43a66988 prune: scan and unlink already pruned block files on startup (Andrew Toth)
Pull request description:
There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since https://github.com/bitcoin/bitcoin/commit/ccd8ef65f93ed82a87cee634660bed3ac17d9eb5).
This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.
ACKs for top commit:
achow101:
ACK 3141eab9c669488a2e7fef5f60d356ac92294922
pablomartin4btc:
re-ACK with added functional test 3141eab9c669488a2e7fef5f60d356ac92294922.
furszy:
Code review ACK 3141eab9
theStack:
Code-review ACK 3141eab9c669488a2e7fef5f60d356ac92294922
Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d
|
|
which sets a maximum value for unspendable outputs.
7013da07fbcddb04abae9759767a9419ab90444c Add release note for PR#25943 (David Gumberg)
04f270b4358417fc2827b9f91717816062b1864e Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg)
Pull request description:
This PR adds a user configurable, zero by default parameter — `maxburnamount` — to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes #25899.
As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions:
1. Those that begin with `OP_RETURN` - (datacarriers)
2. Those whose lengths exceed the script limit.
3. Those that contain invalid opcodes.
The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool.
I see two legitimate use cases for this override:
1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain.
2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
ACKs for top commit:
glozow:
reACK 7013da07fbcddb04abae9759767a9419ab90444c
achow101:
re-ACK 7013da07fbcddb04abae9759767a9419ab90444c
Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
|
|
VerifyDB dosn't finish successfully
0af16e7134459e0820ab95d751093876c1ec4c6d doc: add release note for #25574 (Martin Zumsande)
57ef2a4812f443b2d734f43cebf3ef5038da83f2 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb2540b69564104767d38342704230cbc2 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cfcbc2c2ad5ee289a0642ed00386d013 validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d2675788de5c4a28ea77d823f6d809e validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
ryanofsky:
Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
MarcoFalke:
lgtm re-ACK 0af16e7134459e0820ab95d751093876c1ec4c6d 🎚
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
|
|
c3b4b5a142b204ceeca4e9b1ca1e2ff41ddd1308 test: Replace 0xC0 constant (roconnor-blockstream)
Pull request description:
Instead it should be the named constant `LEAF_VERSION_TAPSCRIPT`.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/27143/commits/c3b4b5a142b204ceeca4e9b1ca1e2ff41ddd1308
theStack:
ACK c3b4b5a142b204ceeca4e9b1ca1e2ff41ddd1308
Tree-SHA512: c00be584ea2d0e7c01bf5620da0da1f37e5b5298ef95df48d91d137c8c542f5d91be158d45392cf2ba8874bf27bd12924e2eed395773b49d091e3028de3356a2
|
|
4bbf5ddd44bde15b328be131922123eaa3212a7e Detailed error message for passphrases with null chars (John Moffett)
b4bdabc2238750a1f6e72cb1403f8b770fc4f365 doc: Release notes for 27068 (John Moffett)
4b1205ba37d6737722d2087696b1a054a852286a Test case for passphrases with null characters (John Moffett)
00a0861181cc7f4771ac2690ca6be5731c30b005 Pass all characters to SecureString including nulls (John Moffett)
Pull request description:
`SecureString` is a `std::string` specialization with a secure allocator. However, in practice it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security.
Instead of assigning to `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and still doesn't make any extraneous copies in memory.
Note to reviewers, the following all compile identically in recent `GCC` (x86-64 and ARM64) with `-O2` (and `-std=c++17`):
```C++
std::string orig_string;
std::cin >> orig_string;
SecureString s;
s.reserve(100);
// The following all compile identically
s = orig_string;
s = std::string_view{orig_string};
s.assign(std::string_view{orig_string});
s.assign(orig_string.data(), orig_string.size());
```
So it's largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory.
Something like `SecureString s{orig_string};` is still invalid and probably unwanted in our case, since it'd get treated as a short string and optimized away from the secure allocator. I presume that's the reason for the `reserve()` calls.
Fixes #27067.
ACKs for top commit:
achow101:
re-ACK 4bbf5ddd44bde15b328be131922123eaa3212a7e
stickies-v:
re-ACK [4bbf5dd](https://github.com/bitcoin/bitcoin/commit/4bbf5ddd44bde15b328be131922123eaa3212a7e)
furszy:
utACK 4bbf5ddd
Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7
|
|
4d84eaec82e7b5a450d47cd30e5936a717035f77 Raise PRNG seed log to INFO. (roconnor-blockstream)
Pull request description:
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log (stdout/stderr) of the failed build.
For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.
By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
ACKs for top commit:
MarcoFalke:
lgtm ACK 4d84eaec82e7b5a450d47cd30e5936a717035f77
theStack:
ACK 4d84eaec82e7b5a450d47cd30e5936a717035f77
Tree-SHA512: 3ccb4a4e7639a3babc3b2a6456a6d0bffc090da34e4545b317f7bfbed4e9950d1b38ea5b2a90c37ccb49b3454bdeff03a6aaf86770b9c4dd14b26320aba50b94
|
|
passphrase to migratewallet
9486509be65f09174a0cb50a337cac58a0c09de4 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow)
aaf02b5721a8b5d3d9280dc3146fa5e44ea671b6 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow)
7fd125b27d48e410509f3009e2eb9fa5cd6729dd wallet: Be able to unlock the wallet for migration (Andrew Chow)
6bdbc5ff590de18dfb47c31190baad879f68fef7 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow)
dbfa34540372033d95036a02b7025ddd33f540aa wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow)
Pull request description:
`migratewallet` currently operates on wallets that are already loaded, however this is not necessarily required, and in the future, not possible once the legacy wallet is removed. So we need to also be able to give the wallet name to migrate.
Additionally, the passphrase is required when migrating a wallet. Since a wallet may not be loaded when we migrate, and as we currently unload wallets when migrating, we need the passphrase to be given to `migratewallet` in order to migrate encrypted wallets.
Fixes #27048
ACKs for top commit:
john-moffett:
reACK 9486509be65f09174a0cb50a337cac58a0c09de4
pinheadmz:
ACK 9486509be65f09174a0cb50a337cac58a0c09de4
furszy:
ACK 9486509b
Tree-SHA512: 35e2ba69a148e129a41e20d7fb99c4cab7947b1b7e7c362f4fd06ff8ac6e79e476e07207e063ba5b80e1a33e2343f4b4f1d72d7930ce80c34571c130d2f5cff4
|
|
Instead it should be the named constant `LEAF_VERSION_TAPSCRIPT`.
|
|
61bb4e783b3acc62b121a228f6b14c2462e23315 lint: enable E722 do not use bare except (Leonardo Lazzaro)
Pull request description:
Improve test code and enable E722 lint check.
If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).
Reference: https://peps.python.org/pep-0008/#programming-recommendations
ACKs for top commit:
MarcoFalke:
lgtm ACK 61bb4e783b3acc62b121a228f6b14c2462e23315
Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
|
|
|
|
Add a functional test to make sure the system
properly accepts passphrases with null characters.
|
|
needed for rescanning
6a5b348f2e526f048d0b448b01f6c4ab608569af test: test rescanning encrypted wallets (ishaanam)
493b813e171a389a8b6750b4f2e42e8363a0267e wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam)
66a86ebabb26a055ca92af846bfa39dbd2f9f722 wallet: keep track of when the passphrase is needed when rescanning (ishaanam)
Pull request description:
Wallet passphrases are needed to top up the keypool of encrypted wallets
during a rescan. The following RPCs need the passphrase when rescanning:
- `importdescriptors`
- `rescanblockchain`
The following RPCs use the information about whether or not the
passphrase is being used to ensure that full rescans are able to
take place (meaning the following RPCs should not be able to run
if a rescan requiring the wallet to be unlocked is taking place):
- `walletlock`
- `encryptwallet`
- `walletpassphrasechange`
`m_relock_mutex` is also introduced so that the passphrase is not
deleted from memory when the timeout provided in
`walletpassphrase` is up and the wallet is still rescanning.
Fixes #25702, #11249
Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions.
ACKs for top commit:
achow101:
ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af
hernanmarino:
ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af
furszy:
Tested ACK 6a5b348f
Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5
|
|
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log of the failed build.
For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.
By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.
|
|
sendrawtransaction.
'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.
|
|
with avoidpartialspends
14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27051
When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain.
If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected.
I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582
ACKs for top commit:
achow101:
ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e
Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
|
|
to allow inferring descriptors for scripts larger than 520 bytes
73ec4b2a8347c796b9aadc1f2576b286c469f9e7 tests: decodescript can infer descriptors for scripts >520 bytes (Andrew Chow)
7cc78223710679c6e7fd40b762798a1f5ca4938e rpc: Use FlatSigningProvider in decodescript (Andrew Chow)
Pull request description:
`FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded.
Fixes #27111
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/27113/commits/73ec4b2a8347c796b9aadc1f2576b286c469f9e7
Tree-SHA512: c0e6d21025e2da864471989ac94c54e127d05459b9b048f34a0da8d76d8e372d5472a2e667ba2db74d6286e3e6faa55486ffa9232a068b519afa676394031d5a
|
|
`p2p_disconnect_ban`
1819564c2130d4d8537ca433c6688b56c769fb79 test: fix intermittent issue in `p2p_disconnect_ban` (brunoerg)
Pull request description:
Fixes #26808
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
ACKs for top commit:
MarcoFalke:
lgtm ACK 1819564c2130d4d8537ca433c6688b56c769fb79
Tree-SHA512: 53a386fc38e2faa6f6da3536e76857ff4b6f55e2590d73fe857b3fe5d0f3ff92c5c7e4abd50ab4be250cb2106a4d14ad95d4809ea60c6e00ed3ac0e71255b0b0
|
|
(and `--timeout-factor 0`)
14302a4802e2dbb41f5189de88f99ddd5dda7736 test: fix test abort for high timeout values (and `--timeout-factor 0`) (Sebastian Falbesoner)
Pull request description:
On master, the functional tests's option `--timeout-factor 0` (which according to the test docs and parameter description should disable the RPC timeouts) currently fails, same as high values like `--timeout-factor 999999`:
```
$ ./test/functional/wallet_basic.py --timeout-factor 0
2022-08-29T01:26:39.561000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f24yxzp5
2022-08-29T01:26:40.262000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/honey/bitcoin/test/functional/test_framework/test_framework.py", line 549, in start_nodes
node.wait_for_rpc_connection()
File "/home/honey/bitcoin/test/functional/test_framework/test_node.py", line 234, in wait_for_rpc_connection
rpc.getblockcount()
File "/home/honey/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
File "/home/honey/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
self.__conn.request(method, path, postdata, headers)
File "/usr/local/lib/python3.9/http/client.py", line 1285, in request
self._send_request(method, url, body, headers, encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1331, in _send_request
self.endheaders(body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1280, in endheaders
self._send_output(message_body, encode_chunked=encode_chunked)
File "/usr/local/lib/python3.9/http/client.py", line 1040, in _send_output
self.send(msg)
File "/usr/local/lib/python3.9/http/client.py", line 980, in send
self.connect()
File "/usr/local/lib/python3.9/http/client.py", line 946, in connect
self.sock = self._create_connection(
File "/usr/local/lib/python3.9/socket.py", line 844, in create_connection
raise err
File "/usr/local/lib/python3.9/socket.py", line 832, in create_connection
sock.connect(sa)
OSError: [Errno 22] Invalid argument
```
This is caused by a high timeout value that Python's HTTP(S) client library can't cope with. Fix this by clamping down the connection's set timeout value in AuthProxy. The change can easily be tested by running an arbitrary test with `--timeout-factor 0` on master (should fail), on this PR (should pass) and on this PR with the clamping value increased by 1 (should fail).
// EDIT: The behaviour was observed on OpenBSD 7.1 and Python 3.9.12.
ACKs for top commit:
MarcoFalke:
lgtm ACK 14302a4802e2dbb41f5189de88f99ddd5dda7736
Tree-SHA512: 6469e8ac699f1bb7dea11d5fb8b3ae54d895bb908570587c5631144cd41fe980ca0b1e6d0b7bfa07983307cba15fb26ae92e6766375672bf5be838d8e5422dbc
|
|
When `node0` calls `disconnectnode` to disconnect `node1`, we should check in `node1` if it worked, because for `node0` the informations in `getpeerinfo` may be updated before really completing the disconnection.
|
|
|
|
|
|
if -connect is specified
2555a3950f0304b7af7609c1e6c696993c50ac72 p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta)
Pull request description:
If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted.
Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`.
Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`.
If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration.
ACKs for top commit:
mzumsande:
Code Review ACK 2555a3950f0304b7af7609c1e6c696993c50ac72
achow101:
ACK 2555a3950f0304b7af7609c1e6c696993c50ac72
vasild:
ACK 2555a3950f0304b7af7609c1e6c696993c50ac72
Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
|
|
Now the verifychain RPC returns false if the checks didn't
finish because the blocks requested to be queried have been pruned.
|
|
4c8ecccdcd813fac3a7ef6a1493ef3977220421d test: add tests for `outputs` argument to `bumpfee`/`psbtbumpfee` (Seibart Nedor)
c0ebb9838252fb187db8719755801758d89651f7 wallet: add `outputs` arguments to `bumpfee` and `psbtbumpfee` (Seibart Nedor)
a804f3cfc0b4761b9ca7976e6e4472cd93599bbf wallet: extract and reuse RPC argument format definition for outputs (Seibart Nedor)
Pull request description:
This implements a modification of the proposal in #22007: instead of **adding** outputs to the set of outputs in the original transaction, the outputs given by `outputs` argument **completely replace** the outputs in the original transaction.
As noted below, this makes it easier to "cancel" a transaction or to reduce the amounts in the outputs, which is not the case with the original proposal in #22007, but it seems from the discussion in this PR that the **replace** behavior is more desirable than **add** one.
ACKs for top commit:
achow101:
ACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d
1440000bytes:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/25344/commits/4c8ecccdcd813fac3a7ef6a1493ef3977220421d
ishaanam:
reACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d
Tree-SHA512: 31361f4a9b79c162bda7929583b0a3fd200e09f4c1a5378b12007576d6b14e02e9e4f0bab8aa209f08f75ac25a1f4805ad16ebff4a0334b07ad2378cc0090103
|
|
Since migration reloads the wallet, the wallet will always be locked
unless the passphrase is given. migratewallet can now take the
passphrase in order to unlock the wallet for migration.
|
|
|
|
`-maxuploadtarget`
7a83aa0982dbe1c5e89b904f636840cb2ef56357 test: add coverage for unparsable `-maxuploadtarget` (brunoerg)
Pull request description:
This PR adds test coverage for the following error:
https://github.com/bitcoin/bitcoin/blob/7386da7a0b08cd2df8ba88dae1fab9d36424b15c/src/init.cpp#L1096-L1099
Top commit has no ACKs.
Tree-SHA512: c115b2b4d2d0eb2316bf9fafd7e0046aa18c9650062779b3a82d6145d188765bff5317f4ca5f79607732fde6d83e1f67756ac20a12c98d060ee68d8acc20c76e
|
|
741908afc1f9ed2040c18667c75665b300c5dfe7 test: previous releases: add v24.0.1 (Sebastian Falbesoner)
Pull request description:
The same procedure as every release (see dba123167236a172d2d33861d58aa94a19729671 [v23.0] and d8b705f1caeb3b4a6790cb26e4e5584ca791d965 [v22.0]), only a little simpler now: thanks to #25650, the previous release fetch script defaults to downloading/building the necessary tags, i.e. we don't need to extend the tag list in the CI scripts and test/README.md anymore.
ACKs for top commit:
Sjors:
tACK 741908afc1f9ed2040c18667c75665b300c5dfe7
Tree-SHA512: a5426e989bd0bba42aa13e7d4cf60f792bf36bd9a6cdb6ef5799f7574d9a8a20979244627bbd0c6219630367e7fd73bac9e677814bc50233f64592ad035e713e
|
|
6c7a17a8e0eec377f83ed1399f003ae70b898270 psbt: support externally provided preimages for Miniscript satisfaction (Antoine Poinsot)
840a396029316896beda46600aec3c1af09a899c qa: add a "smart" Miniscript fuzz target (Antoine Poinsot)
17e3547241d593bc92c5c6b36c54284d9d9f3feb qa: add a fuzz target generating random nodes from a binary encoding (Antoine Poinsot)
611e12502a5887ffb751bb92fadaa334d484824b qa: functional test Miniscript signing with key and timelocks (Antoine Poinsot)
d57b7f2021d2369f6e88cdf0f562aab27c51beaf refactor: make descriptors in Miniscript functional test more readable (Antoine Poinsot)
0a8fc9e200b5018c1efd6f9126eb405ca0beeea3 wallet: check solvability using descriptor in AvailableCoins (Antoine Poinsot)
560e62b1e221832ae99ff8684559a7b8f9df84a7 script/sign: signing support for Miniscripts with hash preimage challenges (Antoine Poinsot)
a2f81b6a8f1ff3b0750711409c7538812a52ef40 script/sign: signing support for Miniscript with timelocks (Antoine Poinsot)
61c6d1a8440db09c44d7fd367a6f2c641ea93d40 script/sign: basic signing support for Miniscript descriptors (Antoine Poinsot)
4242c1c52127df3a24be0c15b88d4fc463af04fc Align 'e' property of or_d and andor with website spec (Pieter Wuille)
f5deb417804b9f267830bd40177677987df4526d Various additional explanations of the satisfaction logic from Pieter (Pieter Wuille)
22c5b00345063bdeb8b6d3da8b5692d18f92bfb7 miniscript: satisfaction support (Antoine Poinsot)
Pull request description:
This makes the Miniscript descriptors solvable.
Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR.
ACKs for top commit:
achow101:
ACK 6c7a17a8e0eec377f83ed1399f003ae70b898270
sipa:
utACK 6c7a17a8e0eec377f83ed1399f003ae70b898270 (to the extent that it's not my own code).
Tree-SHA512: a71ec002aaf66bd429012caa338fc58384067bcd2f453a46e21d381ed1bacc8e57afb9db57c0fb4bf40de43b30808815e9ebc0ae1fbd9e61df0e7b91a17771cc
|
|
mempool_updatefromblock.py by using MiniWallet
dee8549be39f841a24c3c8a3af3c5f787b9ad880 test: simplify and speedup mempool_updatefromblock.py by using MiniWallet (Sebastian Falbesoner)
Pull request description:
This PR simplifies the functional test mempool_updatefromblock.py by using MiniWallet in order to avoid manual low-level tx creation (signing, outputs selection, fee calculation). Most of the tedious work is done by the method `MiniWallet.send_self_transfer_multi` (calling `create_self_transfer_multi` internally) which supports spending a given set of UTXOs and creating a certain number of outputs.
As a nice side-effect, the test's performance increases significantly (~3.5x on my system):
```
master
1m56.80s real 1m50.10s user 0m06.36s system
PR
0m32.34s real 0m30.26s user 0m01.41s system
```
The arguments `start_input_txid` and `end_address` have been removed from the `transaction_graph_test` method, as they are currently unused and I don't see them being needed for future tests.
ACKs for top commit:
brunoerg:
crACK dee8549be39f841a24c3c8a3af3c5f787b9ad880
MarcoFalke:
lgtm ACK dee8549be39f841a24c3c8a3af3c5f787b9ad880 🚏
Tree-SHA512: 9f6da634bdc8c272f9a2af1cddaa364ee371d4e95554463a066249eecebb668d8c6cb123ec8a5404c41b3291010c0c8806a8a01dd227733cec03e73aa93b0103
|
|
|
|
|
|
invalid proof-of-work disconnects peer
772671245d50d94fd5087deb2542854604eba174 test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner)
Pull request description:
One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`):
https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2752-L2762
The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer:
https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2368-L2372
This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty.
ACKs for top commit:
Sjors:
ACK 772671245d50d94fd5087deb2542854604eba174
achow101:
ACK 772671245d50d94fd5087deb2542854604eba174
brunoerg:
crACK 772671245d50d94fd5087deb2542854604eba174
furszy:
Code review ACK 77267124 with a non-blocking speedup.
Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11
|
|
|
