[pull] master from bitcoin:master #1424
Merged
Conversation
This file contains hidden or 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
Just don't call this function when it won't have any effect. Note that we can't remove the LookupBlockIndex call, since `last_received_header` is needed to check if new headers were received (`received_new_header`).
Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Co-Authored-By: Pieter Wuille <pieter@wuille.net>
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it.
No need to pass consensusParams, as CheckHeadersPoW already has access to m_chainparams.GetConsensus() Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
Co-Authored-By: Aurèle Oulès <aurele@oules.com>
Also mark CompressedHeader as explicit, and GetFullHeader as const Co-Authored-By: Aurèle Oulès <aurele@oules.com>
chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer Also slightly reformat HeaderSyncState constructor to make clang-format happy Lastly, remove `const` from `chain_start` declaration in headers_sync_chainwork_tests, to work aroud a false-positive dangling-reference warning in gcc 13.0 Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
Adds -regtest flag to enable testing with regtest chain parameters.
Check mempool exists before accessing size when prev_chainstate doesn't have initialized mempool.
Adds functional test coverage for bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot
This makes sure the initial address self-announcement a node sends to a peer happends in a separate P2P message. This has benefits for both inbound and outbound connections: For inbound connections from a peer to us, previously, we might send the self-announcement along with our response to a GETADDR request. However, the self-announcement might replace an address from the GETADDR response. This isn't clean. For outbound connections from us to a peer, previously, it could have happend that we send the self-announcement along with other addresses. Since shortly after connection open, the peer might only have one rate-limiting token for us, and the addresses are shuffeld on arrival, it's possible that the self-announcement gets rate-limited. However, note that these rate-limitings seem to be rare in practice. This is inspired by and based on #33699 (comment) Co-Authored-By: Anthony Towns <aj@erisian.com.au>
Use port=0 for dynamic port allocation in test framework components to avoid "address already in use" errors from concurrent tests or ports stuck in TIME_WAIT state from previous test runs. Changes: - socks5.py: Update conf.addr after bind() to reflect actual port - p2p.py: Retrieve actual port after create_server() when port=0 - feature_proxy.py: Use port=0 for all SOCKS5 proxy servers - feature_anchors.py: Use port=0 for onion proxy server
-BEGIN VERIFY SCRIPT- sed --in-place --regexp-extended 's/BENCHMARK\(([^,]+), benchmark::PriorityLevel::(HIGH|LOW)\)/BENCHMARK(\1)/g' $( git grep -l PriorityLevel ) sed --in-place 's/#define BENCHMARK(n, priority_level)/#define BENCHMARK(n)/g' ./src/bench/bench.h -END VERIFY SCRIPT-
Duplicate benchmarks with the same name are not supported. Expanding the name with __LINE__ is confusing and brittle, because it makes duplication bugs silent. Fix this twofold: * By enforcing unique benchmarks at compile-time and link-time. For example, a link failure may now look like: "mold: error: duplicate symbol: bench_runner_AddrManAdd" * By enforcing unique benchmarks at run-time. This should never happen, due to the build-failure, but a failure may look like: "Assertion `benchmarks().try_emplace(std::move(name), std::move(func)).second' failed."
This makes the code more consistent. Also, use "using BenchFunction = ..." while touching the header. Also, fixup the whitespace after and earlier scripted-diff.
de4242f refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni) e37555e refactor: Use initializer list in CompressedHeader (Daniela Brozzoni) 0488bdf refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni) 256246a refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni) ca0243e refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille) 4568652 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni) 4066bfe refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni) 0bf6139 p2p: Avoid an IsAncestorOfBestHeaderOrTip call (Pieter Wuille) Pull request description: This is a partial* revival of #25968 It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717: - Avoid an IsAncestorOfBestHeaderOrTip call: Just don't call this function when it won't have any effect. - Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible. - Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it. It also contains the following code cleanups, which were suggested by reviewers in #25968: - Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus() - Remove unused parameter in ReportHeadersPresync - Use initializer list in CompressedHeader, also make GetFullHeader const - Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer *I decided to leave out three commits that were in #25968 (4e7ac7b, ab52fb4, 7f1cf44), since they're a bit more involved, and I'm a new contributor. If this PR gets merged, I'll comment under #25968 to note that these three commits are still up for grabs :) ACKs for top commit: l0rinc: ACK de4242f polespinasa: re-ACK de4242f sipa: ACK de4242f achow101: ACK de4242f hodlinator: re-ACK de4242f Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
8fb5e5f test: check wallet rescan properly in feature_pruning (brunoerg) 9b57c8d test: fix feature_pruning when built without wallet (brunoerg) Pull request description: Fixes #34175 In `feature_pruning`, the`wallet_test` doesn't require any specific wallet functionality and this test is important for one of next ones (`test_scanblocks_pruned`). The reason is that it synchronizes the node 5 and, without this sync, `test_scanblocks_pruned` will fail since we expect `scanblocks` to fail due to `Block not available (pruned data)` and it doesn't happen without this sync. ACKs for top commit: achow101: ACK 8fb5e5f furszy: utACK 8fb5e5f musaHaruna: Tested ACK [8fb5e5f](8fb5e5f) w0xlt: ACK 8fb5e5f Tree-SHA512: 812afbf4343a7493e2169eb6735fce25692d5cb19972abafc772b8c05a64b9c7027f6675cd084f345977e916e62a722d671f90831bbdc51683e0cd253fa482f0
…age 🎄 792e2ed p2p: first addr self-announcement in separate msg (0xb10c) Pull request description: This makes sure the initial address self-announcement a node sends to a peer happends in a separate P2P message. This has benefits for both inbound and outbound connections: For inbound connections from a peer to us, previously, we might send the self-announcement along with our response to a GETADDR request. However, the self-announcement might replace an address from the GETADDR response. This isn't clean. For outbound connections from us to a peer, previously, it could have happend that we send the self-announcement along with other addresses. Since shortly after connection open, the peer might only have one rate-limiting token for us, and the addresses are shuffeld on arrival, it's possible that the self-announcement gets rate-limited. However, note that these rate-limitings seem to be rare in practice. This is inspired by and based on #33699 (comment). The discussion there should be helpful for reviewers. ACKs for top commit: bensig: ACK 792e2ed achow101: ACK 792e2ed fjahr: Code review ACK 792e2ed frankomosh: Code Review ACK [792e2ed](792e2ed) Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260
…onality 7b5d256 test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech) 2bc3265 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech) 5f3d6bd Add regtest support to bitcoin-chainstate tool (stringintech) Pull request description: This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot. The PR also includes: - Fix for assertion crash in `ChainstateManager::AddChainstate()` when `prev_chainstate` has no initialized mempool (required for the test to pass) - `-regtest` flag support for bitcoin-chainstate to enable the testing This work started while experimenting with the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage. ACKs for top commit: achow101: ACK 7b5d256 theStack: Concept and code-review ACK 7b5d256 sedited: Re-ACK 7b5d256 Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
ce63d37 test: use dynamic port allocation to avoid test conflicts (woltx) Pull request description: Use `port=0` for dynamic port allocation in test framework components to avoid intermittent "address already in use" errors when running tests concurrently or when ports are stuck in TIME_WAIT state. Example: #29415 (comment) Changes: - Update `socks5.py` and `p2p.py` to support dynamic port allocation - Convert `feature_proxy.py` and `feature_anchors.py` to use `port=0` ACKs for top commit: achow101: ACK ce63d37 vasild: ACK ce63d37 mzumsande: re-ACK ce63d37 Tree-SHA512: 4efcedca3bde209fbd1bdc2a4ae04b7d53515587d86e421ce61064f78c675c71b45d9782b514c5e7cfc0e92842c947d49f7a3fddb03fe619fcdec9b565f0ecbd
fa3df52 bench: Require semicolon after BENCHMARK(foo) (MarcoFalke) fa8938f bench: Remove incorrect __LINE__ in BENCHMARK macro (MarcoFalke) fa51a28 scripted-diff: Remove priority_level from BENCHMARK macro (MarcoFalke) fa790c3 bench: Remove -priority-level= option (MarcoFalke) Pull request description: The option was added in #26158, when the project was using an autotools-based build system. However, in the meantime this option is unused: * First, commit 27f1121 removed the option from one CI task * Then #32310 removed the option from CMakeList.txt, because: * they only run as a sanity check (fastest version) * no one otherwise runs them, not even CI * issues have been missed due to this Finally, after commit 0ad4376, I don't see a single reason to keep this option, so remove it. Also, there is a commit to turn a silent ignore of duplicate bench names into an error. ACKs for top commit: achow101: ACK fa3df52 l0rinc: ACK fa3df52 hebasto: re-ACK fa3df52, only suggested changes since my recent [review](#34210 (review)). Tree-SHA512: 68a314bff551fa878196d5a615d41d71e1c8c504135e6fc555659aa9f0c8786957d49ba038448e933554a8bc54caea2ddd7d628042c5627bf3bf37628210f8fb
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
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] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )