[pull] master from bitcoin:master #116
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
The coinbase check is repeated again early during ProcessNewBlock. Pre-checking it may also shadow more fundamental problems with a block. In most cases the block header is checked first, before validating the transactions. Checking the coinbase first therefore masks potential issues with the header. Fix this by removing the pre-check. The pre-check was likely introduced on top of ada0caa to fix UB in GetWitnessCommitmentIndex in case a block's transactions are empty. This code path could only be reached because of the call to UpdateUncommittedBlockStructures in submitblock, but cannot be reached through net_processing. Add some functional test cases to cover the previous conditions that lead to a "Block does not start with a coinbase" json rpc error being returned. --- With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden. The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.
ProcessNewBlock fails if an invalid duplicate block is passed in through its call to AcceptBlock and AcceptBlockHeader. The failure in AcceptBlockHeader makes AcceptBlock return early. This makes the pre-check in submitblock redundant. --- With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden. The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.
The duplicate checks are repeated early in the contextual checks of ProcessNewBlock. If duplicate blocks are detected much of their validation is skipped. Depending on the constitution of the block, validating the merkle root of the block is part of the more intensive workload when validating a block. This could be an argument for moving the pre-checks into block processing. In net_processing this would have a smaller effect however, since the block mutation check, which also validates the merkle root, is done before. A side effect of this change is that a duplicate block is persisted again on disk even when pruning is activated. This is similar to the behaviour with getblockfrompeer. Add a release note for this change in behaviour. Testing spamming a node with valid, but duplicate unrequested blocks seems to exhaust a CPU thread, but does not seem to significantly impact keeping up with the tip. The benefits of adding these checks to net_processing are questionable, especially since there are other ways to trigger the more CPU-intensive checks without submitting a duplicate block. Since these DOS concerns apply even less to the RPC interface, which does not have banning mechanics built in, remove them too. --- With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden. The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.
This tests the new submitblock behaviour that is introduced in the previous commit: Submitting a previously pruned block should persist the block's data again.
The behaviour of submitblock was changed in the previous commit, so change it here too.
a0eafc1 functional test: Deduplicate assert_mempool_contents() (Hodlinator) Pull request description: Recently added `mempool_util` implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e8 (related comments: #30239 (comment), #31279 (review)). ACKs for top commit: instagibbs: ACK a0eafc1 achow101: ACK a0eafc1 l0rinc: ACK a0eafc1 theStack: ACK a0eafc1 Tree-SHA512: 25ea807d7c041c18be0e4f424131419365d7c1e0fc6c4fb7ac7289c2f8196fd341ff2a2a3ea88df2c3a389edb4571a5fb889efc1b0204c65f7e09ef8f608d0d3
The `-ffile-prefix-map` compiler option implies `-fprofile-prefix-map` on GCC or `-fcoverage-prefix-map` on Clang, which can lead to issues with coverage builds. This change applies only the options necessary for build reproducibility and accurate source location messages.
We build the only moreutils utility we actually need (sponge), have less unused stuff in the Guix environment, and, the dependency graph is simplified. i.e we no-longer have a dependency on perl, docbook etc, for this package.
Can be tested by running ``` $ sudo tcpdump -i eth0 host 11.22.33.44 ``` and verifying that no packets appear in the tcpdump output. Co-authored-by: Vasil Dimov <[email protected]>
This change resolves build issues that occur when the source or build directory is symlinked.
This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda. The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with: /opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const': /home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)'
466e4df assert_mempool_contents: assert not duplicates expected (Greg Sanders) ea5db2f functional: only generate required blocks for test (Greg Sanders) d033acb fuzz: package_eval: let fuzzer run out input in main tx creation loop (Greg Sanders) ba35a57 CheckEphemeralSpends: return boolean, and set child state and txid outparams (Greg Sanders) cf0cee1 func: add note about lack of 1P1C propagation in tree submitpackage (Greg Sanders) 8424290 unit test: ephemeral_tests is using a dust relay rate, not minrelay (Greg Sanders) d9cfa5f CheckEphemeralSpends: no need to iterate inputs if no parent dust (Greg Sanders) 87b26e3 func: rename test_free_relay to test_no_minrelay_fee (Greg Sanders) e5709a4 func: slight elaboration on submitpackage restriction (Greg Sanders) 08e969b RPC: only enforce dust rules on priority when standardness active (Greg Sanders) ca050d1 unit test: adapt to changing MAX_DUST_OUTPUTS_PER_TX (Greg Sanders) 7c34901 fuzz: package_eval: move last_tx inside txn ctor (Greg Sanders) 445eaed fuzz: use optional status instead of should_rbf_eph_spend (Greg Sanders) 4dfdf61 fuzz: remove unused TransactionsDelta validation interface (Greg Sanders) 09ce926 func: cleanup reorg test comment (Greg Sanders) 768a0c1 func: cleanup test_dustrelay comments (Greg Sanders) bedca1c fuzz: Directly place transactions in vector (Greg Sanders) c041ad6 fuzz: explain package eval coin tracking better (Greg Sanders) bc0d98e fuzz: remove dangling reference to GetEntry (Greg Sanders) 15b6cbf unit test: make dust index less magical (Greg Sanders) 5fbcfd1 unit test: assert txid returned on CheckEphemeralSpends failures (Greg Sanders) ef94d84 bench: remove unnecessary CMTxn constructors (Greg Sanders) c5c10fd ephemeral policy doxygen cleanup (Greg Sanders) dd9044b ephemeral policy: IWYU (Greg Sanders) c6859ce Move+rename GetDustIndexes -> GetDust (Greg Sanders) 62016b3 Use std::ranges for ephemeral policy checks (Greg Sanders) 3ed930a Have HasDust and PreCheckValidEphemeralTx take CTransaction (Greg Sanders) 04a614b Rename CheckValidEphemeralTx to PreCheckEphemeralTx (Greg Sanders) cbf1a47 CheckEphemeralSpends: only compute txid of tx when needed (Greg Sanders) Pull request description: Follow-up to #30239 Here are the parent PR's comments that should be addressed by this PR: https://github.com/bitcoin/bitcoin/pull/30239/files#r1834529646 https://github.com/bitcoin/bitcoin/pull/30239/files#r1831247308 https://github.com/bitcoin/bitcoin/pull/30239/files#r1832622481 https://github.com/bitcoin/bitcoin/pull/30239/files#r1831195216 #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) #30239 (comment) ACKs for top commit: naumenkogs: ACK 466e4df hodlinator: ACK 466e4df theStack: lgtm ACK 466e4df glozow: utACK 466e4df Tree-SHA512: 89106f695755c238b84e0996b89446c0733e10a94c867f656d516d26697d2efe38dfc332188b8589a0a26a3d2bd2c88c6ab70c108e187ce5bfcb91bbf3fb0391
…size The prevector benchmarks were likely not trying to measure vector resize performance.
* Since the main LIMITED_WHILE stated `outpoints.size() < 200'000`, I've presized outpoints accordingly. * `tx_mut.vin` and `tx_mut.vout` weren't caught by the clang-tidy, but addressed them anyway.
And under the hood suppoert single transactions in AcceptPackage. This simplifies user experience and paves the way for reducing number of codepaths for transaction acceptance in the future. Co-Authored-By: instagibbs <[email protected]>
Use the word "disconnecting" everywhere for easier grep.
This is not a pure refactor: 1. It slightly changes the log messages, as reflected in the test changes 2. It adds the IP address to all disconnect logging (when fLogIPs is set)
…ake 2) b031b79 [ci] Split out native fuzz jobs for macOS and windows (dergoegge) Pull request description: Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora. In both jobs the fuzz binary is built with `-DBUILD_FOR_FUZZING` to enable `Assume` assertions as well as `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. ACKs for top commit: maflcko: re-lgtm ACK b031b79 achow101: ACK b031b79 hebasto: ACK b031b79. Tree-SHA512: 7d0dc5a9cb299f6f4596dd9a5526b6aaf82efc6eba308bdc9d8b0a45f79dea87204fb6cd4b2ea2a1bd952466b2e958d64021999296d110d7a83c1667f4de51fe
… definition c288c79 interpreter: Use the same type for SignatureHash in the definition (TheCharlatan) Pull request description: This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda. The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with: ``` /opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_consensus.a(interpreter.cpp.o): in function `GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const': /home/user/bitcoin/build_kernel_riscv/./script/interpreter.cpp:2043:(.text._ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion[_ZNK34GenericTransactionSignatureCheckerI12CTransactionE19CheckECDSASignatureERKSt6vectorIhSaIhEES6_RK7CScript10SigVersion]+0xee): undefined reference to `uint256 SignatureHash<CTransaction>(CScript const&, CTransaction const&, unsigned int, int, long long const&, SigVersion, PrecomputedTransactionData const*)' ``` With this patch it is possible to link against the static consensus library and produce a fully static executable. ACKs for top commit: l0rinc: ACK c288c79 maflcko: review ACK c288c79 🐺 achow101: ACK c288c79 theuni: Obvious fix ACK c288c79. BrandonOdiwuor: Code Review ACK c288c79 Tree-SHA512: 74f283637f0a9cd0cab65d3502f2f8fc4fb983c7672f24e7a76ba2eb6e53b4a81cca0aacb610ef39ac0a454305be594ab440a697ae3718987bf5dbcbc7146a31
…-mode" preset e196190 cmake: Remove unused `BUILD_TESTING` variable from "dev-mode" preset (Hennadii Stepanov) Pull request description: On the master branch @ bb57017: ``` $ cmake -B build --preset dev-mode -DWITH_MULTIPROCESS=OFF <snip> -- Configuring done (12.0s) -- Generating done (0.1s) CMake Warning: Manually-specified variables were not used by the project: BUILD_TESTING -- Build files have been written to: /home/hebasto/git/bitcoin/build ``` This PR resolves the issue. The removed `BUILD_TESTING` variable is a part of the [`CTest`](https://cmake.org/cmake/help/latest/module/CTest.html) module, which we do not include in the project. ACKs for top commit: TheCharlatan: ACK e196190 Tree-SHA512: 8110a0f5bdcdd0844ce7dd75160a61d8b3aff95e12da1ec4d55c56c82da41145736da0fad072adeb97551c99e46683a3493435c3bac7d8e4e62ea6086f60fb7a
…mq.py` be1a2e5 doc: Install `py3-zmq` port on OpenBSD for `interface_zmq.py` (Hennadii Stepanov) Pull request description: On OpenBSD, Python's `zmq` module is provided as a separate [port](https://www.ports.to/path/net/py-zmq,python3.html). This PR updates the OpenBSD Build Guide to include this port, enabling the `interface_zmq.py` functional test. Also updates the documented OpenBSD version. ACKs for top commit: theStack: Tested ACK be1a2e5 Tree-SHA512: 4d560385b94e8c7491aa19d2157d8a799617e08136601dc565a909d4c74e12582a1d273bc97ad7c2d0e57c5cf7377560ba02ef58c12f8991652322553740d2ba
b871020 guix: disable timezone tools & profiling in glibc (fanquake) 23b8a42 guix: bump glibc 2.31 to 7b27c450c34563a28e634cccb399cd415e71ebfe (fanquake) Pull request description: An additional commit has been backported to the 2.31 branch: https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/release/2.31/master. Pass `--disable-timezone-tools`: removes `var/profiles/x86_64-linux-gnu/sbin/zdump`. Pass `--disable-profile`: profiling is disabled by default, but make that explicit. ACKs for top commit: theuni: utACK b871020 hebasto: ACK b871020. Tree-SHA512: 0d9a0e7451cc42384bbdd0b46c740c7aa964dc12e3f0376de586bf90e57799ebb04675892861cb38a53b5ca0e265061fa7111596cf1c94171303d0d048785ab4
Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit. This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB); Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as: > 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat. > 2024-12-18T18:25:03Z [warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes > 2024-12-18T18:25:09Z Shutdown: done Note that the related BCLog::BENCH units were also converted to `KiB` from `kB` to unify the bases. Co-authored-by: Cory Fields <[email protected]>
The mutex (required by TestBlockValidity) must be held after creating the block, until TestBlockValidity is called. Otherwise, it is possible that the chain advances in the meantime and leads to a crash in TestBlockValidity: Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338) The diff can be reviewed with the git options --ignore-all-space --function-context
Co-Authored-By: David Gumberg <[email protected]>
If AssumeUtxo background sync is completed in this ActivateBestChain() call, the GetRole() function returns "normal" instead of "background" for this chainstate. This would make the wallet (which ignores BlockConnected notifcation for the background chainstate) process it, change m_last_block_processed_height, and display an incorrect balance.
Use a third node for this, which doesn't get restarted like the second node. This test would fail without the previous commit.
…ct logging 06443b8 net: clarify if we ever sent or received from peer (Sjors Provoost) 1d01ad4 net: add LogIP() helper, use in net_processing (Sjors Provoost) 937ef9e net_processing: use CNode::DisconnectMsg helper (Sjors Provoost) ad22442 net: additional disconnection logging (Sjors Provoost) Pull request description: While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful. All cases where we disconnect now come with a log message that has the word `disconnecting`: * all calls to `CloseSocketDisconnect()` log `disconnecting peer=…` * wherever we set `pnode->fDisconnect = true;` * for all `InactivityCheck` cases (which in turn sets `fDisconnect`) * replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…` A few exceptions are listed here: #28521 (comment) I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so. This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though. The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use. Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`. ACKs for top commit: davidgumberg: reACK 06443b8 vasild: ACK 06443b8 danielabrozzoni: ACK 06443b8 hodlinator: ACK 06443b8 Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
5709718 coins: warn on shutdown for big UTXO set flushes (Lőrinc) Pull request description: Split out of #30611 (comment) Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit. This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB); Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as: > 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat. > 2024-12-18T18:25:03Z [warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes > 2024-12-18T18:25:09Z Shutdown: done --- You can reproduce it by starting `bitcoind` with a large `-dbcache`: > mkdir demo && cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bitcoind -datadir=demo **-dbcache=10000** Waiting until the used memory is over 1 GiB > 2024-12-18T18:25:02Z UpdateTip: [...] progress=0.069009 cache=**1181.1MiB**(8827981txo) And cancelling the process from the terminal: > ^C2024-12-18T18:25:03Z tor: Thread interrupt > [...] > 2024-12-18T18:25:03Z **[warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes* ACKs for top commit: sipa: utACK 5709718 tdb3: re ACK 5709718 1440000bytes: ACK 5709718 danielabrozzoni: tACK 5709718 Tree-SHA512: 608cf797de788501ccb2986508c155f5660c5f6f7a414524bfcc2820cfa9ebe3da558d13f2317d1f121a82d49ffe1e711a1152c743c22dab9f9807363f4ed8d5
To get the maximum size of a satisfaction for a descriptor without considering the max sig, the parameter `use_max_sig` should be false.
b9766c9 Remove unused variable assignment (yancy) Pull request description: The variable is conditionally assigned toward the end of the loop and not used after. It's then set back to its default value at the beginning of the loop. ACKs for top commit: theuni: utACK b9766c9 achow101: ACK b9766c9 hodlinator: crACK b9766c9 danielabrozzoni: code review ACK b9766c9 murchandamus: ACK b9766c9 Tree-SHA512: 45e62b0dd561a473f5ae21bfa91db494940b752886669c85b63a83b68d2a157a301e9450082635e921f3dc812e6307f4ad1674806b74b3e7e0f9f4db543ad93d
…d getmininginfo ecaa786 rpc: add signet_challenge field to getblockchaininfo and getmininginfo (Ash Manning) Pull request description: Signet challenges are currently only available via `getblocktemplate` RPC. `getblockchaininfo` and `getmininginfo` both provide inadequate information to distinguish signets. Since these are the RPCs used to determine the current network, they should also provide the signet challenge for signets. Test coverage is included in `test/functional/feature_signet.py`. ACKs for top commit: sipa: utACK ecaa786 achow101: ACK ecaa786 i-am-yuvi: Concept ACK ecaa786 Sjors: ACK ecaa786 zaidmstrr: Tested ACK [ecaa786](ecaa786) Tree-SHA512: 9ccf4ae634ee74353a2a895efb881fdc62ae703a134ccd219da2cd6080c7d38319e689054584722457a7cc79004bd6022292a3b0b90eaab9f7003564665e1ea4
…queue e56fc7c rpc: increase the defaults for -rpcthreads and -rpcworkqueue (Vasil Dimov) Pull request description: `rpcthreads` was introduced with a default of 4 in 2013 in 21eb5ad `rpcworkqueue` was introduced with a default of 16 in 2015 in 40b556d Resolves: #29386 --- Just bump the ancient default values. There is no perfect default that would fit everybody. This could lead to https://bikeshed.com/ ACKs for top commit: achow101: ACK e56fc7c andrewtoth: ACK e56fc7c storopoli: ACK e56fc7c tdb3: ACK e56fc7c Tree-SHA512: ba3ea7392fda57950daa6b4c4d38ecdef9eebe5e786824d25f8b5cea03e760ffff7f77f3acd8eb6c6178b1e92b282e02cabb940ed7222eec7f73efdb819eef06
fa494a1 refactor: Specify const in std::span constructor, where needed (MarcoFalke) faaf480 Allow std::span in stream serialization (MarcoFalke) faa5391 refactor: test: Return std::span from StringBytes (MarcoFalke) fa86223 refactor: Avoid passing span iterators when data pointers are expected (MarcoFalke) faae6fa refactor: Simplify SpanPopBack (MarcoFalke) facc4f1 refactor: Replace fwd-decl with proper include (MarcoFalke) fac3a78 refactor: Avoid needless, unsafe c-style cast (MarcoFalke) Pull request description: The `std::span` type is already used in some parts of the codebase, and in most contexts can implicitly convert to and from `Span`. However, the two types are not identical in behavior and trying to use one over the other can result in compile failures in some contexts. Fix all those issues by allowing either `Span` or `std::span` in any part of the codebase. All of the changes are also required for the scripted-diff to replace `Span` with `std::span` in #31519 ACKs for top commit: sipa: utACK fa494a1 fjahr: Code review ACK fa494a1 achow101: ACK fa494a1 theuni: utACK fa494a1. adamandrews1: utACK fa494a1 Tree-SHA512: 9440941823e884ff5d7ac161f58b9a0704d8e803b4c91c400bdb5f58f898e4637d63ae627cfc7330e98a721fc38285a04641175aa18d991bd35f8b69ed1d74c4
… error a2c45ae test: report failure during utf8 response decoding (furszy) Pull request description: Useful for debugging issues such #31241 (comment). Prints the entire response content instead of printing only the position of the byte it can't be decoded. The diff between the error messages can be seen by running the `wallet_migration.py` functional test with the following patch applied: ``` diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp --- a/src/wallet/rpc/wallet.cpp(revision d65918c) +++ b/src/wallet/rpc/wallet.cpp(date 1731005254673) @@ -801,7 +801,7 @@ } UniValue r{UniValue::VOBJ}; - r.pushKV("wallet_name", res->wallet_name); + r.pushKV("wallet_name", "\xc3\x28"); if (res->watchonly_wallet) { r.pushKV("watchonly_name", res->watchonly_wallet->GetName()); } ``` ACKs for top commit: achow101: ACK a2c45ae theStack: re-ACK a2c45ae rkrux: tACK a2c45ae ismaelsadeeq: utACK a2c45ae Tree-SHA512: 6abb524b5a215c51ec881eea91ebe8174140a88ff3874c8c88676157edae7818801356586a904dbb21b45053183315a6d71dbf917d753611d8e413776b57c484
fa6e599 test: Call generate through test framework only (MarcoFalke) Pull request description: The generate RPCs are special in that they should only be called by the test framework itself. This way, they will call the sync function on the nodes, which can avoid intermittent test issues. Also, when the sync is disabled, it will happen explicitly by setting the `sync_fun`. Apply this rule here, so that all generate calls are written consistently. ACKs for top commit: achow101: ACK fa6e599 rkrux: tACK fa6e599 hodlinator: ACK fa6e599 i-am-yuvi: Tested ACK fa6e599 Tree-SHA512: 31079997f1e17031ecd577904457e0560388aa53cadb1bbda281865271e8e4cf244bc6bf315838a717bf9d6620c201093e30039aa0007bec3629f7ca56abfba3
…ght_new, 248) fa0998f test: Avoid intermittent error in assert_equal(pruneheight_new, 248) (MarcoFalke) Pull request description: Fixes #31446 The test uses the P2P network to sync blocks, which has no inherent guarantee that the blocks are sent and received in the right order, assuming the headers are received first. This can mean that the first block file is flushed with block at height 249 and block at height 248 is added to the second file. In the log it looks like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...249, time=2011-02-02...2024-12-03) (onto 1) (height 248)`. The test assumes that the height of the last pruned block in the first file is 248, expecting it to look like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...248, time=2011-02-02...2024-12-09) (onto 1) (height 249) `. Fix the issue by using a linear dumb sync. ACKs for top commit: achow101: ACK fa0998f mzumsande: Code Review ACK fa0998f i-am-yuvi: Code Review ACK fa0998f fjahr: Code review ACK fa0998f Tree-SHA512: 59cb4317be6cf9012c9bf7a3e9f5ba96b8b114b30bd2ac42af4fe742cd26a634d685b075f04a84bd782b2a43a342d75bb20a042bd82ad2831dbf844d39517ca2
d9d5bc2 qa: Limit `-maxconnections` in tests (Hennadii Stepanov) Pull request description: On systems such as NetBSD, the `bitcoind` typically prints the following warning: ``` Warning: Reducing -maxconnections from 125 to 96, because of system limitations. ``` This breaks the functional test framework (see #23968). This PR limits the `-maxconnections` to mitigate the issue and enable functional tests on NetBSD. Fixes #23968. Here is CI log from the [`bitcoin-core-nightly`](https://github.com/hebasto/bitcoin-core-nightly) repository: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12415868523/job/34663207030 ACKs for top commit: maflcko: re-ACK d9d5bc2 achow101: ACK d9d5bc2 mzumsande: Code Review ACK d9d5bc2 tdb3: tested ACK d9d5bc2 Tree-SHA512: ad02adce98ce609176c9688289a0ca347932da5b6f259c6ab4e686914352f3d61f819adc150be80220f969200ee6a0c1c8737426525816fa0df58d688c51410c
…ompletion bc43eca test: add functional test for balance after snapshot completion (Martin Zumsande) 226d03d validation: Send correct notification during snapshot completion (Martin Zumsande) Pull request description: After AssumeUtxo background sync is completed in a `ActivateBestChain()` call, the `GetRole()` function called with `BlockConnected()` returns `ChainstateRole::NORMAL` instead of `ChainstateRole::BACKGROUND` for this chainstate. This would make the wallet (which ignores `BlockConnected` notifications for the background chainstate) process it, change `m_last_block_processed_height` to the (ancient) snapshot height, and display an incorrect balance. Fix this by caching the chainstate role before calling `ActivateBestChainStep()`. Also contains a test for this situation that fails on master. Fixes #31546 ACKs for top commit: fjahr: re-ACK bc43eca achow101: ACK bc43eca furszy: Code review ACK bc43eca TheCharlatan: lgtm ACK bc43eca Tree-SHA512: c5db677cf3fbab3a33ec127ec6c27c8812299e8368fd3c986bc34d0e515c4eb256f6104479f27829eefc098197de3af75d64ddca636b6b612900a0e21243e4f2
fa63b82 test: generateblocks called by multiple threads (MarcoFalke) fa62c8b rpc: Extend scope of validation mutex in generateblock (MarcoFalke) Pull request description: The mutex (required by TestBlockValidity) must be held after creating the block, until TestBlockValidity is called. Otherwise, it is possible that the chain advances in the meantime and leads to a crash in TestBlockValidity: `Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)` Fixes #31562 ACKs for top commit: davidgumberg: reACK fa63b82 achow101: ACK fa63b82 ismaelsadeeq: re-ACK fa63b82 mzumsande: utACK fa63b82 Tree-SHA512: 3dfda1192af52546ab11fbffe44af8713073763863f4a63fbcdbdf95b1c6cbeb003dc4b8b29e7ec67362238ad15e07d8f6855832a0c68dc5370254f8cbf9445c
b29d68f test: descriptor: fix test for `MaxSatisfactionWeight` (brunoerg) Pull request description: To get the maximum size of a satisfaction for a descriptor with no max sig, the parameter `use_max_sig` should be false. ACKs for top commit: fjahr: utACK b29d68f achow101: ACK b29d68f tdb3: re ACK b29d68f furszy: utACK b29d68f Tree-SHA512: 8559718d126e60ce21a34183f74d227546108b43e3897e49622d6677ed9e7707caa962fd811d8787bd4dafc48a0e779ef11050d5990293faa2f91ded4aaa4f4b
366ae00 descriptor: Assume `ParseScript` is not being called with a P2WPKH context (brunoerg) e366408 descriptor: remove unreachable verification for `pkh` (brunoerg) Pull request description: This PR removes an unreachable verification in the `ParseScript` function. It returns an error if `pkh` is not being used at top level, sh, wsh or tr. However, any usage of `pkh` without these contexts will not reach this verification but other ones like "invalid keys" (e.g. `wpkh(pkh(L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd))`). ACKs for top commit: davidgumberg: crACK 366ae00 achow101: ACK 366ae00 tdb3: cr ACK 366ae00 sipa: crACK 366ae00 Tree-SHA512: b954221a77eed623aeed5eb54f14e82c49540a151d3388831924caa7a784e48a2a975e418af1e13d491e4f8cded3b1797aa39e0e4e39e302a991105df09cdec0
…cept response b6f0593 doc: add release note about testmempoolaccept debug-message (Matthew Zipkin) f9cac63 test: cover testmempoolaccept debug-message in RBF test (Matthew Zipkin) f9650e1 rbf: remove unecessary newline at end of error string (Matthew Zipkin) 221c789 rpc: include verbose reject-details field in testmempoolaccept response (Matthew Zipkin) Pull request description: Adds a new field `reject-details` in `testmempoolaccept` responses to include `m_debug_message` from `ValidationState`. This string is the complete error message thrown by the mempool in response to `sendrawtransaction`. The extra verbosity is helpful to consumers of `testmempoolaccept`, which is sort of a debug tool anyway. example: > > { > "txid": "07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8", > "wtxid": "5dc243b1b92ee2f5a43134eb3e23449be03d1abb3d7f3c03c836ed0f13c50185", > "allowed": false, > "reject-reason": "insufficient fee", > "reject-details": "insufficient fee, rejecting replacement 07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB" > } ACKs for top commit: rkrux: re-ACK b6f0593 glozow: ACK b6f0593 Tree-SHA512: 340b8023d59cefa84598879c4efdb7c399a3f62da126e87c595523f302e53d33098fc69da9c5f8c92b7580dc75466c66cea372051f935b197265648fe15c43a3
fa83bec refactor: Allow std::byte in Read(LE/BE) (MarcoFalke) Pull request description: Starting with C++17, `std::byte` is often (not always) a better choice over `uint8_t` for new code. However, the existing codebase discourages the use of `std::byte`, when helpers such as `ReadLE32` are used. This is because calling code will be cluttered with byte-casts. Fix it by allowing `std::byte` pointers in `ReadLE32` (and friends). ACKs for top commit: sipa: utACK fa83bec fjahr: Code review ACK fa83bec theuni: utACK fa83bec l0rinc: ACK fa83bec Tree-SHA512: 83604dc9df9ad447ad1b6f81f1e1844554c2c5331fcb78bdba1300e050d9dcbe9cf7a1b2dd250772bb23a8bf02a4ec26441012fe2f4bcc670ef31c15151adb15
faf7eac test: clang-format -i src/univalue/test/unitester.cpp (MarcoFalke) fafa9cc test: Embed univalue json tests in binary (MarcoFalke) fa04485 test: Re-enable univalue test fail18.json (MarcoFalke) 63b6b63 build: Use character literals for generated headers to avoid narrowing (Lőrinc) Pull request description: All other benchmarks and tests have their data embedded, except for the univalue json tests. This is not only confusing, but also problematic, when the test binary is moved to a different system for testing, because one has to put the test files in the source dir that was used at compile-time. Fix all issues by embedding them. Also, re-enable a disabled test. Also, fix an issue in the GenerateHeaderFromJson.cmake. Requested in https://github.com/bitcoin/bitcoin/pull/31434/files#r1876000910 ACKs for top commit: l0rinc: ACK faf7eac fjahr: tACK faf7eac achow101: ACK faf7eac TheCharlatan: Re-ACK faf7eac hebasto: Re-ACK faf7eac. The commit, which modifies CMake scripts, has been replaced with the one from #31547, and a formatting commit has been added since my recent [review](#31542 (review)). Tree-SHA512: 72ad202125746f32ccf07411ad3efd2771f27a40525c204cba3c9c83b3ca46d05dd18f6fa5985720c6684bdcbb4c4853fc609ced095ddd1a124832318dd8a55d
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )