Sjors
commented at 5:07 pm on December 24, 2024:
member
Introduce checkBlock() on the Mining interface for IPC, as well as a new checkblock RPC that’s easier to use than getblocktemplate in proposal mode, and supports weak blocks.
Rationale
Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.1 This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation2.
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.3 There’s currently no way for it to do so, short of re-implementing Bitcoin Core validation code.
(although SRI could use this PR, the Template Provider role doesn’t need it, so this is not part of #31098)
Verifying weak blocks (reduced PoW)
Stratum v2 decentralises block template generation, but still hinge on a centralised entity to approve these templates.
There used to be a decentralised mining protocol P2Pool4 which relied on (what we would today call) weak blocks. In such a system all participants perform verification and there is no privileged pool operator (that can reject a template).
P2Pool shares form a “sharechain” with each share referencing the previous share’s hash. Each share contains a standard Bitcoin block header, some P2Pool-specific data that is used to compute the generation transaction (total subsidy, payout script of this share, a nonce, the previous share’s hash, and the current target for shares), and a Merkle branch linking that generation transaction to the block header’s Merkle hash.
IIUC only the header and coinbase of these shares / weak blocks were distributed amongst all pool participants and verified. This was sufficient at the time because fees were negligible, so payouts could simply be distributed based on work.
However, if P2Pool were to be resurrected today5, it would need to account for fees in a similar manner as PPLNS2 above. In that case the share chain should include the full weak blocks6. The client software would need a way to verify those weak blocks.
Enter checkblock and its IPC counterpart checkBlock()
Given a serialised block, it performs the same checks as submitblock, but without updating the block index, chainstate, etc. The proof-of-work check can be bypassed entirely, for the first use of verifying block templates. Alternatively a custom target can be provided for the use case of weak blocks.
When called with check_pow=false the checkblock RPC is (almost) the same as getblocktemplate in proposal mode, and indeed they both call checkBlock() on the Mining interface.
Implementation details:
TestBlockValidity() is moved to the ChainstateManager and refactored to no longer return BlockValidationState. This avoids the new checkBlock() method needing to pass BlockValidationState over IPC.
The refactor commit contains additional changes explained in its commit message.
Another commit drops support for the unused BlockValidationState encoding.
This PR contains a simple unit test and a more elaborate functional test.
Potential followups:
if the block contains “better” transactions, (optionally) add them to our mempool 5
this improves compact block relay once the real block comes in ↩︎↩︎
The share chain client software could use compact block encoding to reduce its orphan / uncle rate. To make that easier, we could provide a reconstructblock RPC (reconstructBlock IPC) that takes the header and short ids, fills in a CBlock from the mempool (and jail) and returns a list of missing items. The share chain client then goes and fetches missing items and calls the method again. It then passes the complete block to checkblock. This avoids the need for others to fully implement compact block relay. Note that even if we implemented p2p weak block relay, it would not be useful for share chain clients, because they need to relay additional pool-specific data. ↩︎
The 'subtractfeefromamount' arg is only boolean for sendtoaddress().
Other commands should never provide it as a boolean.
4f4cd35319
RPC: improve SFFO arg parsing, error catching and coverage
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()
cddcbaf81e
test: complete BDB parser (handle internal/overflow pages, support all page sizes)
This aims to complete our test framework BDB parser to reflect
our read-only BDB parser in the wallet codebase. This could be
useful both for making review of #26606 easier and to also possibly
improve our functional tests for the BDB parser by comparing with
an alternative implementation.
01ddd9f646
test: compare BDB dumps of test framework parser and wallet toold45eb3964f
depends, refactor: Avoid hardcoding `host_prefix` in toolchain file
This change allows the entire `depends/<host_prefix>` directory to be
relocatable.
d9c8aacce3
func test: Expand tx download preference tests
1. Check that outbound nodes are treated
the same as whitelisted connections for
the purposes of getdata delays
2. Add test case that demonstrates
download retries are preferentially
given to outbound (preferred) connections
even when multiple announcements are
considered ready.
846a138728
ci: optionally use local docker build cache
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
indefinitly 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 cache type=local
can be found https://docs.docker.com/build/cache/backends/local/
This replaces https://github.com/bitcoin/bitcoin/pull/31377
e87429a2d0
DrahtBot
commented at 5:07 pm on December 24, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#31649 (consensus: Remove checkpoints (take 2) by marcofleon)
#31283 (Add waitNext() to BlockTemplate interface by Sjors)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Sjors
commented at 5:09 pm on December 24, 2024:
member
I wrote on IRC:
Does anyone know what UpdateUncommittedBlockStructures is good for in the submitblock RPC?
Sjors[m]: it looks like it adds the “witness nonce” if missing (the witness for the coinbase input is a reserved field for adding future commitments, but it must be present, so that function adds a 0)
This PR omits UpdateUncommittedBlockStructures. It would be nice to come up with a test that shows what it does and fails on the current code.
mzumsande
commented at 8:51 pm on December 26, 2024:
contributor
The newly introduced CheckNewBlock() looks very similar to the existing TestBlockValidity() which was just removed from the mining interface in bfc4e029d41ec3052d68f174565802016cb05d41 - it just seems to be a bit more verbose and allows the multiplier for the PoW. Did you consider merging these functions, so that we don’t have multiple functions in validation that basically do the same thing?
in
src/pow.cpp:165
in
9887db317coutdated
160@@ -161,3 +161,19 @@ bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Par
161162 return true;
163 }
164+
165+bool CheckWeakProofOfWork(uint256 hash, unsigned int nBits, unsigned int multiplier, const Consensus::Params& params)
Feel free to ignore before more concept acks arrive.
CheckWeakProofOfWork() seems really similar to CheckProofOfWorkImpl(). Maybe I’m missing something simple. What was the rationale for creating the new function vs adding an argument?
tdb3
commented at 0:21 am on December 27, 2024:
contributor
Concept ACK
Really liking the idea of facilitating these type of systems.
Instead of a multiplier I could also allow a custom target. This would give weak block systems more flexibility on how to derive their difficulty.
I’d be in favor of this. It’s more flexible, could prevent interface churn, and if the user wishes to create the custom target from a simple multiplication they could do the multiplication on their end.
Sjors
commented at 2:32 pm on December 27, 2024:
member
The newly introduced CheckNewBlock() looks very similar to the existing TestBlockValidity()
It looks like I ended up with a similar design after a long detour. I’ll look into combining these.
Instead of a multiplier I could also allow a custom target
@instagibbs is there any reason you went for a multiplier?
Sjors force-pushed
on Dec 30, 2024
instagibbs
commented at 4:47 pm on December 30, 2024:
member
is there any reason you went for a multiplier?
nothing off the top of my head
Sjors
commented at 4:48 pm on December 30, 2024:
member
I refactored checkblock to take a target argument rather than a multiplier. In order to make this easier to implement and test, I also introduced a gettarget RPC and DeriveTarget() helper.
I then dropped the original TestBlockValidity and renamed CheckNewBlock to it. The generateblock and getblocktemplate RPC calls, as well
as BlockAssembler::CreateNewBlock use the new method.
If people prefer I could rewrite the git history to directly refactor TestBlockValidity, though I suspect the current approach is easier to review.
I split the test changes out into #31581, so will mark this PR as draft for now. While working on that I noticed a rather big difference between the original TestBlockValidity and my incarnation of it: the former required holding cs_main, which I think was wrong.
Sjors force-pushed
on Dec 30, 2024
DrahtBot added the label
CI failed
on Dec 30, 2024
DrahtBot
commented at 4:58 pm on December 30, 2024:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Sjors force-pushed
on Dec 30, 2024
Sjors force-pushed
on Dec 30, 2024
Sjors force-pushed
on Dec 30, 2024
Sjors marked this as a draft
on Dec 30, 2024
Sjors
commented at 6:27 pm on December 30, 2024:
member
I might make a separate PR to consider whether to drop AssertLockHeld(cs_main) from the original TestBlockValidity.
If that ends up rejected, then I’ll keep the existing TestBlockValidity calls as they are, i.e. drop the last two commits from this PR and leave them as a followup.
While working on that I noticed a rather big difference between the original TestBlockValidity and my incarnation of it: the former required holding cs_main, which I think was wrong.
Update: I was confused, CheckBlock needs to be called with a lock on cs_main, and there’s no need to “drop AssertLockHeld(cs_main)”. That should allow for some simplifications.
DrahtBot added the label
Needs rebase
on Dec 30, 2024
Sjors force-pushed
on Dec 30, 2024
Sjors
commented at 8:13 pm on December 30, 2024:
member
DrahtBot removed the label
Needs rebase
on Dec 30, 2024
DrahtBot removed the label
CI failed
on Dec 31, 2024
interfaces: Add helper function for wallet on pruning42d5d53363
validation: Don't assume m_chain_tx_count in GuessVerificationProgress
In the context of an a descriptor import during assumeutxo background sync, the progress can not be estimated due to m_chain_tx_count being set to 0.
27f99b6d63
rpc: Improve importdescriptor RPC error messages
Particularly add more details in the case of pruning or assumeutxo.
d73ae603d4
test, assumeutxo: import descriptors during background sync595edee169
rpc: Include assumeutxo as a failure reason of rescanblockchain9d2d9f7ce2
ci: Allow build dir on CI host8888ee4403
gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs
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.
3e97ff9c5e
achow101 referenced this in commit
c506f2cee7
on Jan 6, 2025
DrahtBot added the label
Needs rebase
on Jan 6, 2025
luke-jr
commented at 6:15 am on January 7, 2025:
member
The new RPC interface is redundant with the existing BIP 23 block proposals support.
P.S. DATUM does not provide a way for the pool to reject valid blocks. There is no approval of templates.
Sjors force-pushed
on Jan 7, 2025
Sjors
commented at 8:36 am on January 7, 2025:
member
Rebased after #31581 landed. I also cherry-picked the latest relevant commits from #31583.
@luke-jr I dropped DATUM from the description, since there’s still no spec it’s hard to understand what it actually does.
The checkblock RPC is indeed almost the same as getblocktemplate in proposal mode. I updated the description to point this out. The main difference is that it can check (reduced) proof-of-work.
My long term goal for this new method is to make it more powerful. It could optionally add new transactions to the mempool to improve compact block relay. It could perform small reorgs to validate forks, which can be useful for (stale) share accounting (and perhaps uncle and DAG schemes).
An alternative approach could be drop the checkblock RPC and leave (weak) PoW checking as an IPC-only feature. If people prefer that I can convert the tests in rpc_checkblock.py to instead increase coverage for getblocktemplate.
But I think it’s nice to offer weak block checking support for mining pool projects that want to experiment with it, without forcing them to use IPC. The new RPC method is short and test covered.
(in the above I’m assuming that getblocktemplate should be considered more or less ossified)
Sjors force-pushed
on Jan 7, 2025
DrahtBot added the label
CI failed
on Jan 7, 2025
DrahtBot
commented at 8:52 am on January 7, 2025:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label
Needs rebase
on Jan 7, 2025
Sjors force-pushed
on Jan 7, 2025
Sjors
commented at 11:02 am on January 7, 2025:
member
The weak block test was broken, because regtest uses the almost highest possible target. Using a multiplier would overflow it. I dropped 180ce81182e840f01e6e9534872ebf92647c2e39 and adjusted the tests. I also dropped b61f8cca13320088a83aadef5f6af66375342fae and b74e145bc46e6dbf7d1496f078ff22cf80d8c92a because I don’t need them for the test.
Sjors force-pushed
on Jan 7, 2025
DrahtBot removed the label
CI failed
on Jan 7, 2025
luke-jr
commented at 12:28 pm on January 7, 2025:
member
What would use the “weak block” check that doesn’t need the ability to do it independently from proposals anyway?
While the existing GBT BIPs are Final, the protocol was designed to allow for extensions. If there’s a use case for this feature, doing it that way seems fine
Sjors
commented at 12:45 pm on January 7, 2025:
member
@luke-jr do you mean that every client that wants to call checkblock $block_hex or getblocktemplate '{"mode": "proposal", "data": "$block_hex"}' for a weak block, probably implements its own proof-of-work check? That’s probably true, since it most likely needs the ability to parse the block anyway in order to inspect the coinbase. Compared to that calculating proof-of-work and comparing against a target is trivial.
What would an extension look like to pass check_pow and target arguments?
net_processing: Add missing use of DisconnectMsg
Makes it easier to grep logs for "disconnecting" when investigating disconnections.
0c4954ac7d
net: Specify context in disconnecting log message04b848e482
net: Bring back log message when resetting socket
Useful in case new disconnects creep in which are not using DisconnectMsg().
bbac17608d
refactor: Remove redundant reindex check
The check for whether the block tree db has been wiped before calling
NeedsRedownload() is confusing. The boolean is set in case of a reindex.
It was originally introduced to guard NeedsRedownload in case of a
reindex in #21009. However NeedsRedownload already returns early if the
chain's tip is not loaded. Since that is the case during a reindex, the
pre-check is redundant.
57ba59c0cd
kernel: Move block tree db open to block manager
This commit is done in preparation for the next commit. Here, the block
tree options are moved to the blockmanager options and the block tree is
instantiated through a helper method of the BlockManager, which is
removed again in the next commit.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
7fbb1bc44b
kernel: Move block tree db open to BlockManager constructor
Make the block db open RAII style by calling it in the BlockManager
constructor.
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.
0cdddeb224
ryanofsky referenced this in commit
5acf12bafe
on Jan 22, 2025
Sjors
commented at 8:28 am on January 23, 2025:
member
Rebased after #31583 landed. Ready for review again.
I still think having a new RPC checkblock $block_hex is more convenient and easier to expand than designing an extension for getblocktemplate, but I’m open to suggestions.
I’m rewriting the commit history so the changes are applied directly to TestBlockValidity, rather than first introducing CheckNewBlock and swapping the method.
Sjors force-pushed
on Jan 23, 2025
Sjors marked this as ready for review
on Jan 23, 2025
Sjors marked this as a draft
on Jan 23, 2025
lint: Call lint_scripted_diff from test_runner
Allowing to call the check from the test_runner allows for consistent
error messages and better UX by having a single test_runner for all
checks.
This requires the env var to be set for now. The next commit makes the
commit range optional.
fa673cf344
lint: Move commit range printing to test_runner
Having a single test_runner for all logic improves the consistency and
UX.
fa99728b0c
lint: Call lint_commit_msg from test_runner
Allowing to call the check from the test_runner allows for consistent
error messages. Also, manually setting the commit range is no longer
needed.
faf8fc5487
Sjors force-pushed
on Jan 23, 2025
Sjors
commented at 11:27 am on January 23, 2025:
member
Rewrote history. This should be easier to follow.
fanquake
commented at 11:35 am on January 23, 2025:
member
0[11:30:52.624] /ci_container_base/src/rpc/mining.cpp: In lambda function:
1[11:30:52.624] /ci_container_base/src/rpc/mining.cpp:389:30: error: designator order for field ‘node::BlockCheckOptions::check_merkle_root’ does not match declaration order in ‘const node::BlockCheckOptions’
2[11:30:52.624]389 | if(!miner.checkBlock(block, {.check_pow = false, .check_merkle_root = false}, reason)){3[11:30:52.624] | ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4[11:30:52.627] gmake[2]: *** [src/CMakeFiles/bitcoin_node.dir/build.make:1070: src/CMakeFiles/bitcoin_node.dir/rpc/mining.cpp.o] Error 1
DrahtBot
commented at 11:55 am on January 23, 2025:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot added the label
CI failed
on Jan 23, 2025
Sjors force-pushed
on Jan 23, 2025
Sjors
commented at 12:10 pm on January 23, 2025:
member
Fixed designator order. What cmake flag emits that warnings? Or does it require a more recent clang version? (using AppleClang 16.0)
Also added a commit that drops BlockValidationState for IPC.
Updated the PR description.
Sjors marked this as ready for review
on Jan 23, 2025
Sjors force-pushed
on Jan 23, 2025
Sjors
commented at 1:44 pm on January 23, 2025:
member
Fixed tidy complaint and moved 0d200decdc03bf0cc25a1f1fe4b76756f5cd33ef earlier.
maflcko
commented at 1:49 pm on January 23, 2025:
member
Fixed designator order. What cmake flag emits that warnings? Or does it require a more recent clang version? (using AppleClang 16.0)
Looking at the CI result, macos was the only task that didn’t catch it. You could try the latest Xcode 16.2, but if that doesn’t work, then I don’t know either.
Sjors
commented at 3:49 pm on January 23, 2025:
member
XCode 16.2 also ships with AppleClang 16.0. I didn’t have Xcode installed before on this machine, but doing so made no difference. Not the end of the world though.
DrahtBot removed the label
CI failed
on Jan 23, 2025
test: add validation for gettxout RPC responsefa0232a3e0
test framework, wallet: rename get_scriptPubKey method to get_output_script723440c5b8
net: Switch to DisconnectMsg in CConnman551a09486c
test: Added coverage to the waitfornewblock rpc
Added a test for the Negative timeout error if the rpc is given a
negative value for its timeout arg
93747d934b
refactor: add GetMinimumTime() helper
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.
0713548137
rpc: clarify BIP94 behavior for curtime79d45b10f1
rpc: have mintime account for timewarp rule
Previously in getblocktemplate only curtime took the timewarp rule into account.
Mining pool software could use either, though in general it should use curtime.
0082f6acc1
doc: release notese1676b08f7
DrahtBot added the label
Needs rebase
on Jan 29, 2025
Sjors force-pushed
on Jan 29, 2025
Sjors
commented at 1:47 pm on January 29, 2025:
member
DrahtBot removed the label
Needs rebase
on Jan 29, 2025
test: fix intermittent timeout in p2p_1p1c_network.py
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.
152a2dcdef
test: fixes p2p_ibd_txrelay wait time
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
1973a9e4f1
test: make sure we are on sync with a peer before checking if they have sent a message
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
3f4b104b1b
Merge bitcoin/bitcoin#31633: net: Disconnect message follow-ups to #28521
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 https://github.com/bitcoin/bitcoin/commit/551a09486c495e1a3cfc296eafdf95e914856bff
achow101:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
danielabrozzoni:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
Tree-SHA512: 95ab8e7436e20ca3abc949ea09697facb6fbeb19981ddc7e0bf294e7ec914e72cbf836c21184a2a887f04cb264f26daf5b0cbcbebc9db633a7b1672b4e488063
1d6c6e98c1
Merge bitcoin/bitcoin#30125: test: improve BDB parser (handle internal/overflow pages, support all page sizes)
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
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 [https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e](https://github.com/bitcoin/bitcoin/pull/30844/commits/cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e)
ismaelsadeeq:
Code review and Tested ACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
Tree-SHA512: c9c15582b81101a93987458d155394ff2c9ca42864624c034ee808a31c3a7d7f55105dea98e86fce17d3c7b2c1a6b5b77942da66b287f8b8881a60cde78c1a3c
c7869cb214
Merge bitcoin/bitcoin#31545: ci: optionally use local docker build cache
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
6835e9686c
[refactor] move parent inv-adding to OrphanResolutionCandidate
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.
57221ad979
[refactor] rename to OrphanResolutionCandidate to MaybeAdd*6e4d392a75
[refactor] make GetCandidatePeers take uint256 and in-out vector
The txrequest fuzzer uses uint256s, not transactions, so it's best if
GetCandidatePeers takes that as an input.
7704139cf0
[fuzz] GetCandidatePeersc4cc61db98
multi-announcer orphan handling test fixups18820ccf6b
[refactor] assign local variable for wtxid32eb6dc758
[doc] how unique_parents can be emptye3bd51e4b5
pass P2PTxInvStore init args to P2PInterface init2da46b88f0
test fix: make peer who sends MSG_TX announcement non-wtxidrelay
Otherwise, it is not meaningful to test whether the announcement is
ignored, because *all* announcements of this type are ignored.
4c1fa6b28c
[p2p] assign just 1 random announcer in AddChildrenToWorkSet7426afbe62
Merge bitcoin/bitcoin#31751: test: fix intermittent timeout in p2p_1p1c_network.py
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
809d7e763c
Merge bitcoin/bitcoin#31428: ci: Allow build dir on CI host
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 https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0
hebasto:
re-ACK 8888ee4403fa62972c49e18752695d15fd32c0b0.
Tree-SHA512: a1e2c32bc1b95efbd0b48287ac5b49e0e1bacbf5a5800845be5352bbdd3e17fa478e90348b2e94e95cf3ae863cdf75ab444089376588f6f8eec438f73a4b5b97
8fa10edcd1
Merge bitcoin/bitcoin#31600: rpc: have getblocktemplate mintime account for timewarp
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
992f37f2e1
Merge bitcoin/bitcoin#31746: test: Added coverage to the waitfornewblock rpc
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
eaf4b928e7
Merge bitcoin/bitcoin#30965: kernel: Move block tree db open to block manager
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
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](https://github.com/bitcoin/bitcoin/pull/29370/commits/0fd915ee6bef63bb360ccc5c039a3c11676c38e3) 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
85f96b01b7
Merge bitcoin-core/gui#850: psbt: Use SIGHASH_DEFAULT when signing PSBTs
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
1172bc4157
test: added additional coverage to waitforblock and waitforblockheight rpc's7e0db87d4f
A bpftrace script that logs information from the
net:*_connection tracepoints.
I've tested this script with bpftrace version 0.14.1 and v0.20.2.
b19b526758
tracing: document that peer addrs can be >68 chars
A v3 onion address with a `:` and a five digit port has a length of
68 chars. As noted in
https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1781040991
peers e.g. added via hostname might have a longer CNode::m_addr_name.
These might be cut off in tracing tools.
e3622a9692
Armss9936 approved
Merge bitcoin/bitcoin#31653: lint: Call more checks from test_runner
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](https://github.com/bitcoin/bitcoin/pull/31653/commits/faf8fc5487d409eeff7b7b260eabb6929a7b7a5f)
willcl-ark:
tACK faf8fc5487d409eeff7b7b260eabb6929a7b7a5f
Tree-SHA512: 78018adc618d997508c226c9eee0a4fada3899cdfd91587132ab1c0389aea69127bafc3a900e90e30aca2c6bae9dcd6e6188ef287e91413bc63ee66fb078b1af
7426afbe62414fa575f91b4f8d3ea63bcc653e8b [p2p] assign just 1 random announcer in AddChildrenToWorkSet (glozow)
4c1fa6b28c292dcaf11d605e0e8c2bbf59cc4de4 test fix: make peer who sends MSG_TX announcement non-wtxidrelay (glozow)
2da46b88f09ff3c58c94bd258273c04d16c8b649 pass P2PTxInvStore init args to P2PInterface init (glozow)
e3bd51e4b52069d1db5c478aaec00666fc8f4101 [doc] how unique_parents can be empty (glozow)
32eb6dc758a90b6c154d1e3e503f0d4840c44b02 [refactor] assign local variable for wtxid (glozow)
18820ccf6b2163b42ee1256d33cc3d14d268b682 multi-announcer orphan handling test fixups (glozow)
c4cc61db98ff1f0a5943fc7469adf9d9df6fddcd [fuzz] GetCandidatePeers (glozow)
7704139cf0dbddf322eac2af9be0c3f2838bc285 [refactor] make GetCandidatePeers take uint256 and in-out vector (glozow)
6e4d392a7536d9c5e1afc60196ee82195dbfec35 [refactor] rename to OrphanResolutionCandidate to MaybeAdd* (glozow)
57221ad97971d399a90d509c5e7b64227f6b2b5e [refactor] move parent inv-adding to OrphanResolutionCandidate (glozow)
Pull request description:
Followup to #31397.
Addressing (in order):
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906077380
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1881060842
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905994963
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905999581
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906001592
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905989913
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905920861
https://github.com/bitcoin/bitcoin/pull/31658#pullrequestreview-2551617694
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1917559601
ACKs for top commit:
instagibbs:
reACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b
marcofleon:
reACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b
mzumsande:
Code Review ACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b
dergoegge:
Code review ACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b
Tree-SHA512: bca8f576873fdaa20b758e1ee9708ce94e618ff14726864b29b50f0f9a4db58136a286d2b654af569b09433a028901fe6bcdda68dcbfea71e2d1271934725503
94ca99ac51
Merge bitcoin/bitcoin#31358: depends: Avoid hardcoding `host_prefix` in toolchain file
d9c8aacce38ab593ea9277976eb64ccadd7d062f depends, refactor: Avoid hardcoding `host_prefix` in toolchain file (Hennadii Stepanov)
Pull request description:
This PR allows the entire `depends/<host_prefix>` directory to be relocatable.
Only `libevent` package configuration files are non-relocatable for the version `2.1.12-stable` we use now. However, this issue has been fixed upstream in https://github.com/libevent/libevent/commit/1f1593ff27bdf51c3e1c45b92cfb0ac931960298 and friends.
Fixes https://github.com/bitcoin/bitcoin/issues/31050.
ACKs for top commit:
theuni:
Neat. utACK d9c8aacce38ab593ea9277976eb64ccadd7d062f
ryanofsky:
Code review ACK d9c8aacce38ab593ea9277976eb64ccadd7d062f
Tree-SHA512: c4c340722e63fc1da36fba2b15f025a6e1d477da1991194d678f546f2f3ea9454e2f0ff054aae6ae6c332a0781a597c3ce63b9018b46b8c258033f0d19efbef3
2aa7be1744
Merge bitcoin/bitcoin#31784: test: added additional coverage to waitforblock and waitforblockheight rpc's
7e0db87d4fff996c086f6e86b62338c98ef30c55 test: added additional coverage to waitforblock and waitforblockheight rpc's (kevkevinpal)
Pull request description:
Similar to https://github.com/bitcoin/bitcoin/pull/31746
This adds test coverage to the `waitforblock` and `waitforblockheight` rpc's by adding a test to assert we get an rpc error if we include a negative timeout
ACKs for top commit:
Sjors:
utACK 7e0db87d4fff996c086f6e86b62338c98ef30c55
Prabhat1308:
ACK [7e0db87](https://github.com/bitcoin/bitcoin/pull/31784/commits/7e0db87d4fff996c086f6e86b62338c98ef30c55)
brunoerg:
code review ACK 7e0db87d4fff996c086f6e86b62338c98ef30c55
BrandonOdiwuor:
Code Review ACK 7e0db87d4fff996c086f6e86b62338c98ef30c55
Tree-SHA512: c3b1b3a525e91e0092b783d73d2401126e3b8792a394be00374258f20cf3d619498e6625d3aad5e5ced29509c5eac828ee03c4ee66ba405b91e89be7e8b07311
846a1387280fa584f70ccb1f5d0198339b065528 func test: Expand tx download preference tests (Greg Sanders)
Pull request description:
1. Check that outbound nodes are treated the same as whitelisted connections for
the purposes of `getdata` delays
2. Add test case that demonstrates download retries are preferentially
given to outbound (preferred) connections
even when multiple announcements are
considered ready.
`NUM_INBOUND` is a magic number large enough that it should fail over 90% of the time
if the underlying outbound->preferred->PriorityComputer logic was broken. Bumping this
to 100 peers cost another 14 seconds locally for the sub-test, so I made it pretty small.
ACKs for top commit:
i-am-yuvi:
tACK 846a1387280fa584f70ccb1f5d0198339b065528 good catch
maflcko:
ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕
marcofleon:
lgtm ACK 846a1387280fa584f70ccb1f5d0198339b065528
Tree-SHA512: 337aa4dc33b5c0abeb4fe7e4cd5e389f7f53ae25dd991ba26615c16999872542391993020122fd255af4c7163f76c1d1feb2f2d6114f12a364c0360d4d52b8c3
1334ca6c07
Merge bitcoin/bitcoin#30226: test: add validation for gettxout RPC response
723440c5b8eb3a815c80bfb37ad195b5448b25ed test framework, wallet: rename get_scriptPubKey method to get_output_script (Alfonso Roman Zubeldia)
fa0232a3e07ad6d11b4d4aaec93e9531ac3274f3 test: add validation for gettxout RPC response (Alfonso Roman Zubeldia)
Pull request description:
Added a new test in `test/functional/rpc_blockchain.py` to validate the gettxout RPC response. This new test ensures all response elements are verified, including `bestblock`, `confirmations`, `value`, `coinbase`, and `scriptPubKey` details.
Also renamed the method `get_scriptPubKey` from `test/functional/test_framework/wallet.py` to the modern name `get_output_script` as suggested by maflcko (https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1925491846)
ACKs for top commit:
fjahr:
reACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed
maflcko:
lgtm ACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed
brunoerg:
code review ACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed
Tree-SHA512: 3384578909d2e7548cef302c5b8a9fed5b82dfc942892503ad4a05e73f5cceafad1eab3af9a27e54aef3db7631f8935298d6b882c70d2f02a9a75b8e3c209b6c
e3622a969293feea75cfadc8f7c6083edcd6d8de tracing: document that peer addrs can be >68 chars (0xb10c)
b19b526758f055733e1c21809cf975169fdd39b0 tracing: log_p2p_connections.bt example (0xb10c)
caa5486574baf805b36c8abc873554aee4ba82b7 tracing: connection closed tracepoint (0xb10c)
b2ad6ede95ea66e8677b31d614e183953966db54 tracing: add misbehaving conn tracepoint (0xb10c)
68c1ef4f19bc4ebe0ca1af95ac378732c4fe6d22 tracing: add inbound connection eviction tracepoint (0xb10c)
4d61d52f4387622701cdad4bb0fb115127021106 tracing: add outbound connection tracepoint (0xb10c)
85b2603eec634257cd3b398900dbb92251db71e6 tracing: add inbound connection tracepoint (0xb10c)
Pull request description:
This adds five new tracepoints with documentation and tests for network connections:
- established connections with `net:inbound_connection` and `net:outbound_connection`
- closed connections (both closed by us or by the peer) with `net:closed_connnection`
- inbound connections that we choose to evict with `net:evicted_inbound_connection`
- connections that are misbehaving and punished with `net:misbehaving_connection`
I've been using these tracepoints for a few months now to monitor connection lifetimes, re-connection frequency by IP and netgroup, misbehavior, peer discouragement, and eviction and more. Together with the two existing P2P message tracepoints they allow for a good overview of local P2P network activity. Also sort-of addresses https://github.com/bitcoin/bitcoin/pull/22006#discussion_r636775863.
I've been back and forth on which arguments to include. For example, `net:evicted_connection` could also include some of the eviction metrics like e.g. `last_block_time`, `min_ping_time`, ... but I've left them out for now. If wanted, this can be added here or in a follow-up. I've tried to minimize a potential performance impact by measuring executed instructions with `gdb` where possible (method described [here](https://github.com/bitcoin/bitcoin/pull/23724#issuecomment-996919963)). I don't think a few hundred extra instructions are too crucial, as connection opens/closes aren't too frequent (compared to e.g. P2P messages). Note: e.g. `CreateNodeFromAcceptedSocket()` usually executes between 80k and 90k instructions for each new inbound connection.
| tracepoint | instructions |
|----------------------------|--------------------------------------------------------|
| net:inbound_connection | 390 ins |
| net:outbound_connection | between 700 and 1000 ins |
| net:closed_connnection | 473 ins |
| net:evicted_inbound_connection | not measured; likely similar to net:closed_connnection |
| net:misbehaving_connection | not measured |
Also added a bpftrace (tested with v0.14.1) `log_p2p_connections.bt` example script that produces output similar to:
```
Attaching 6 probes...
Logging opened, closed, misbehaving, and evicted P2P connections
OUTBOUND conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, total_out=1
INBOUND conn from 127.0.0.1:45324: id=1, type=inbound, network=0, total_in=1
MISBEHAVING conn id=1, message='getdata message size = 50001'
CLOSED conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, established=1231006505
EVICTED conn to 127.0.0.1:45324: id=1, type=inbound, network=0, established=1612312312
```
ACKs for top commit:
laanwj:
re-ACK e3622a969293feea75cfadc8f7c6083edcd6d8de
vasild:
ACK e3622a969293feea75cfadc8f7c6083edcd6d8de
sipa:
utACK e3622a969293feea75cfadc8f7c6083edcd6d8de
Tree-SHA512: 1032dcac6fe0ced981715606f82c2db47016407d3accb8f216c978f010da9bc20453e24a167dcc95287f4783b48562ffb90f645bf230990e3df1b9b9a6d4e5d0
a43f08c4ae
ci: Remove no longer needed '-Wno-error=documentation'f1d7a6dfa1
Merge bitcoin/bitcoin#31760: test: make sure we are on sync with a peer before checking if they have sent a message
3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08 test: make sure we are on sync with a peer before checking if they have sent a message (Sergi Delgado Segura)
Pull request description:
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.
An easy way to check this is the case is to modify one of the test cases to force a request within the expected time, and check how the request is not seen. After the change, the test would crash as expected:
```diff
index 963d92485c..30ab5f2035 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -186,9 +185,12 @@ class OrphanHandlingTest(BitcoinTestFramework):
parent_inv = CInv(t=MSG_WTX, h=int(tx_parent_arrives["tx"].getwtxid(), 16))
assert_equal(len(peer_spy.get_invs()), 0)
peer_spy.assert_no_immediate_response(msg_getdata([parent_inv]))
+ txid = 0xdeadbeef
+ peer_spy.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=txid)]))
# Request would be scheduled with this delay because it is not a preferred relay peer.
self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY)
+ peer_spy.assert_never_requested(int(txid))
peer_spy.assert_never_requested(int(tx_parent_arrives["txid"], 16))
peer_spy.assert_never_requested(int(tx_parent_doesnt_arrive["txid"], 16))
# Request would be scheduled with this delay because it is by txid.
```
It is worth noting that this is not seen in the cases where the message is expected to be received, because in such cases `assert_never_requested` is always after a `wait_....` method, which is already waiting for the node to sync on their end.
ACKs for top commit:
i-am-yuvi:
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08
instagibbs:
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08
glozow:
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08
Tree-SHA512: 321a6605d630bed2217b6374e999dbb84da14138263dd8adf65fe3a6cd7981a50c873beced9cf05cb6d747a912e91017c58e7d4323d25449c87d83095ff4cba9
ae9eaa063b
Merge bitcoin/bitcoin#31759: test: fixes p2p_ibd_txrelay wait time
1973a9e4f1dfba57051135d6e1bca80979de3879 test: fixes p2p_ibd_txrelay wait time (Sergi Delgado Segura)
Pull request description:
`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.
This can be checked by modifying the test so the peer **is not doing IBD**, and checking how the test succeeds on that assert (even though it fails later on, given the nature of the test):
```diff
index 882f5b5c13..3a69ae5860 100755
--- a/test/functional/p2p_ibd_txrelay.py
+++ b/test/functional/p2p_ibd_txrelay.py
@@ -34,7 +34,7 @@ NORMAL_FEE_FILTER = Decimal(100) / COIN
class P2PIBDTxRelayTest(BitcoinTestFramework):
def set_test_params(self):
- self.setup_clean_chain = True
+ # self.setup_clean_chain = True
self.num_nodes = 2
self.extra_args = [
["-minrelaytxfee={}".format(NORMAL_FEE_FILTER)],
@@ -43,9 +43,11 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
def run_test(self):
self.log.info("Check that nodes set minfilter to MAX_MONEY while still in IBD")
- for node in self.nodes:
- assert node.getblockchaininfo()['initialblockdownload']
- self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
+ # for node in self.nodes:
+ # assert node.getblockchaininfo()['initialblockdownload']
+ # self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
```
ACKs for top commit:
i-am-yuvi:
ACK 1973a9e4f1dfba57051135d6e1bca80979de3879
glozow:
ACK 1973a9e4f1dfba57051135d6e1bca80979de3879
Tree-SHA512: c4b3afe9927c5480671ebf5c1f6ee5fc7e3aeefeb13c210fa83587a6c126e1a8e40ad8e46587537d0f4bf06a36bbf2310ca065d685d4d9286e5a446b8d5b2235
82ba505134
Update Transifex slug for 29.x
Update the Transifex slug to match the new resource created for the
upcoming 29.x branch.
2b51dd384b
Merge bitcoin/bitcoin#31804: ci: Remove no longer needed `-Wno-error=documentation`
f1d7a6dfa1411ccf741fbf7351ea4f229dd1e63e ci: Remove no longer needed '-Wno-error=documentation' (Hennadii Stepanov)
Pull request description:
Picked from https://github.com/bitcoin/bitcoin/pull/31726.
ACKs for top commit:
maflcko:
lgtm ACK f1d7a6dfa1411ccf741fbf7351ea4f229dd1e63e
Tree-SHA512: 2c4b197da1a60662922341062f9c1fe43b0e35a50194f4757057a48d7538f2f68540ed56508193a5c6a81e3f7dfbca78b15e3c101aae08d8ccd1fc5df0535932
Note to self: to maintain existing behavior this should emit LogError(). That’s not necessary for generateblock since it already throws.
Emitting LogError() in TestBlockValidity would be wrong as of this PR, because that function doesn’t know if we’re checking our own blocks (which would indicate a fatal problem) or someone else’s.
Actually it’s not needed. This code is for the proposal mode, where we verify a block given to us. It’s not our node error if this block is invalid.
When we generate a block template ourselves through getblocktemplate or createNewBlock(), it goes through BlockAssembler::CreateNewBlock() which by default runs TestBlockValidity at the end, and throws if this return an error. No need to log if we’re already crashing with std::runtime_error.
cmake: Ensure generated sources are up to date for `translate` target
Some sources might be generated, and while they likely do not contain
any translatable strings, this change generalizes the approach to
include generated sources in the translation process as well.
864386a744
qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file
Merge bitcoin/bitcoin#31809: Prepare "Open Transifex translations for v29.0" release step
2f27c910869e301b7e7669e81a0878da64e49957 qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
864386a7444fb5cf16613956ce8bf335f51b67d5 cmake: Ensure generated sources are up to date for `translate` target (Hennadii Stepanov)
2b51dd384b4a2655ee066e8bccd254270d0f5f6c Update Transifex slug for 29.x (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](https://github.com/bitcoin/bitcoin/blob/864386a7444fb5cf16613956ce8bf335f51b67d5/doc/release-process.md).
It is required to open Transifex translations for v29.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/31029.
The previous similar PR: https://github.com/bitcoin/bitcoin/pull/30548.
**Notes for reviewers:**
1. This is the first release process conducted after migrating the build system to CMake. This revealed a bug, which is fixed in the second commit
2. To reproduce the diff in the third commit, follow these steps:
```
gmake -C depends -j $(nproc) MULTIPROCESS=1
cmake --preset dev-mode --toolchain depends/$(./depends/config.guess)/toolchain.cmake
cmake --build build_dev_mode --target translate
```
ACKs for top commit:
stickies-v:
ACK 2f27c910869e301b7e7669e81a0878da64e49957
Tree-SHA512: 325ce2418f218b82cc3b0a6c727473963455680cdf6383a85768613ed9e485974b2e52bd5b2e7a7472ad8ebe40bccb2884764d7f9e83dc10a587cd7892e0028b
f93d6cb0ca
validation: refactor TestBlockValidity
A later commit adds checkBlock() to the Mining interface. In order to
avoid passing BlockValidationState over IPC, this commit first
refactors TestBlockValidity to return a boolean instead, and pass failure
reasons via a string.
TestBlockValidity is moved into ChainstateManager which allows some
simplifications.
Comments are expanded.
The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.
Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.
Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.
0131f11f5a
ipc: drop BlockValidationState special handling
The Mining interface avoids using BlockValidationState.
158373285d
Add checkBlock to Mining interface
And use it in miner_tests, getblocktemplate and generateblock.
dbb59aebd9
rpc: add checkblock15d841e41b
Add target arg to checkblock RPC and IPC41e13ee105
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-02-22 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
