perf: pre-calculate hash of header once#7320
Draft
knst wants to merge 12 commits into
Draft
Conversation
BACKPORT NOTE: partial due to missing changes in bitcoin-chainstate.cpp 3add234 ui: show header pre-synchronization progress (Pieter Wuille) 738421c Emit NotifyHeaderTip signals for pre-synchronization progress (Pieter Wuille) 376086f Make validation interface capable of signalling header presync (Pieter Wuille) 93eae27 Test large reorgs with headerssync logic (Suhas Daftuar) 3555473 Track headers presync progress and log it (Pieter Wuille) 03712dd Expose HeadersSyncState::m_current_height in getpeerinfo() (Suhas Daftuar) 150a548 Test headers sync using minchainwork threshold (Suhas Daftuar) 0b6aa82 Add unit test for HeadersSyncState (Suhas Daftuar) 83c6a0c Reduce spurious messages during headers sync (Suhas Daftuar) ed6cddd Require callers of AcceptBlockHeader() to perform anti-dos checks (Suhas Daftuar) 551a8d9 Utilize anti-DoS headers download strategy (Suhas Daftuar) ed47094 Add functions to construct locators without CChain (Pieter Wuille) 84852bb Add bitdeque, an std::deque<bool> analogue that does bit packing. (Pieter Wuille) 1d4cfa4 Add function to validate difficulty changes (Suhas Daftuar) Pull request description: New nodes starting up for the first time lack protection against DoS from low-difficulty headers. While checkpoints serve as our protection against headers that fork from the main chain below the known checkpointed values, this protection only applies to nodes that have been able to download the honest chain to the checkpointed heights. We can protect all nodes from DoS from low-difficulty headers by adopting a different strategy: before we commit to storing a header in permanent storage, first verify that the header is part of a chain that has sufficiently high work (either `nMinimumChainWork`, or something comparable to our tip). This means that we will download headers from a given peer twice: once to verify the work on the chain, and a second time when permanently storing the headers. The p2p protocol doesn't provide an easy way for us to ensure that we receive the same headers during the second download of peer's headers chain. To ensure that a peer doesn't (say) give us the main chain in phase 1 to trick us into permanently storing an alternate, low-work chain in phase 2, we store commitments to the headers during our first download, which we validate in the second download. Some parameters must be chosen for commitment size/frequency in phase 1, and validation of commitments in phase 2. In this PR, those parameters are chosen to both (a) minimize the per-peer memory usage that an attacker could utilize, and (b) bound the expected amount of permanent memory that an attacker could get us to use to be well-below the memory growth that we'd get from the honest chain (where we expect 1 new block header every 10 minutes). After this PR, we should be able to remove checkpoints from our code, which is a nice philosophical change for us to make as well, as there has been confusion over the years about the role checkpoints play in Bitcoin's consensus algorithm. Thanks to Pieter Wuille for collaborating on this design. ACKs for top commit: Sjors: re-tACK 3add234 mzumsande: re-ACK 3add234 sipa: re-ACK 3add234 glozow: ACK 3add234 Tree-SHA512: e7789d65f62f72141b8899eb4a2fb3d0621278394d2d7adaa004675250118f89a4e4cb42777fe56649d744ec445ad95141e10f6def65f0a58b7b35b2e654a875 Co-authored-by: fanquake <fanquake@gmail.com>
…tidy fixup 6b24dfe CBlockLocator: performance-move-const-arg Clang tidy fixups (Jon Atack) Pull request description: Fix Clang-tidy CI errors on master. See https://cirrus-ci.com/task/4806752200818688?logs=ci#L4696 for an example. ACKs for top commit: MarcoFalke: review ACK 6b24dfe vasild: ACK 6b24dfe Tree-SHA512: 7a67acf7b42da07b63fbb392236e9a7be8cf35c36e37ca980c4467fe8295c2eda8aef10f41a1e3036cd9ebece47fa957fc3256033f853bd6a97ce2ca42799a0a Co-authored-by: MacroFake <falke.marco@gmail.com>
94af3e4 Fix typo from PR25717 (Suhas Daftuar) e5982ec Bypass headers anti-DoS checks for NoBan peers (Suhas Daftuar) 132ed7e Move headerssync logging to BCLog::NET (Suhas Daftuar) Pull request description: Remove BCLog::HEADERSSYNC and move all headerssync logging to BCLog::NET. Bypass headers anti-DoS checks for NoBan peers Also fix a typo that was introduced in PR25717. ACKs for top commit: Sjors: tACK 94af3e4 ajtowns: ACK 94af3e4 sipa: ACK 94af3e4 naumenkogs: ACK 94af3e4 w0xlt: ACK bitcoin@94af3e4 Tree-SHA512: 612d594eddace977359bcc8234b2093d273fd50662f4ac70cb90903d28fb831f6e1aecff51a4ef6c0bb0f6fb5d1aa7ff1eb8798fac5ac142783788f3080717dc Co-authored-by: fanquake <fanquake@gmail.com>
…th_minchainwork.py 88e7807 test: fix non-determinism in p2p_headers_sync_with_minchainwork.py (Suhas Daftuar) Pull request description: The test for node3's chaintips (added in PR25960) needs some sort of synchronization in order to be reliable. ACKs for top commit: mzumsande: Code Review ACK 88e7807 satsie: ACK 88e7807 Tree-SHA512: 5607c5b1a95d91e7cf81b695eb356b782cbb303bcc7fd9044e1058c0c0625c5f9e5fe4f4dde9d2bffa27a80d83fc060336720f7becbba505ccfb8a04fcc81705 Co-authored-by: fanquake <fanquake@gmail.com>
fa4ba04 fuzz: Remove no-op call to get() (MacroFake) fa64228 fuzz: Avoid timeout in bitdeque fuzz target (MacroFake) Pull request description: I'd guess that any bug should be discoverable within `10` ops. However, `900` seems also better than no limit at all, which causes timeouts such as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50892 ACKs for top commit: sipa: ACK fa4ba04 Tree-SHA512: f6bd25e78d5f04c6f88e9300c2fa3d0993a0911cb0fd1b414077adc0edde1a06ad72af5e2f50f0ab1324f91999ae57d879686c545b2e6c19ae7f637a8804bd48 Co-authored-by: fanquake <fanquake@gmail.com>
…eader bdcafb9 p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane) Pull request description: Follow-up to bitcoin#25717. The commit "Utilize anti-DoS headers download strategy" changed how this bool variable is computed, so that its value is now the opposite of what it should be. Prior to bitcoin#25717: ``` bool received_new_header{WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash()) == nullptr)}; ``` After bitcoin#25717 (simplified): ``` { LOCK(cs_main); last_received_header = m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash()); } bool received_new_header{last_received_header != nullptr}; ``` ACKs for top commit: dergoegge: ACK bdcafb9 glozow: ACK bdcafb9, I believe this is correct and don't see anything to suggest the switch was intentional. stickies-v: ACK bdcafb9 Tree-SHA512: 35c12762f1429585a0b1c15053e310e83efb28c3d8cbf4092fad9fe81c893f6d766df1f2b20624882acb9654d0539a0c871f587d7090dc2a198115adf59db3ec Co-authored-by: glozow <gloriajzhao@gmail.com>
…eturn value correctly when new headers sync is started 7ad15d1 [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge) Pull request description: This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit. The issue is that we ignore the return value on [the first `IsContinuationOfLowWorkHeadersSync` call after a new headers sync is started](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/net_processing.cpp#L2553-L2568), which leads to us passing headers to [`ProcessNewBlockHeaders`](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/net_processing.cpp#L2856) when that initial `IsContinuationOfLowWorkHeadersSync` call returns `false`. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different `nBits` value than the prior headers (which fails the pre-sync logic [here](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/headerssync.cpp#L189)). Those 2000 headers will be passed to `ProcessNewBlockHeaders`. I haven't included a test here so far because we can't test this without changing the default value for `CRegTestParams::consensus.fPowAllowMinDifficultyBlocks` or doing some more involved refactoring. ACKs for top commit: sipa: ACK 7ad15d1 glozow: ACK 7ad15d1 Tree-SHA512: 9aabb8bf3700401e79863d0accda0befd2a83c4d469a53f97d827e51139e2f826aee08cdfbc8866b311b153f61fdac9b7aa515fcfa2a21c5e2812c2bf3c03664 Co-authored-by: glozow <gloriajzhao@gmail.com>
784b023 [net processing] Simplify use of IsContinuationOfLowWorkHeadersSync in TryLowWorkHeaderSync (dergoegge) e891aab [net processing] Fixup TryLowWorkHeadersSync comment (dergoegge) Pull request description: See bitcoin#26355 (comment) and bitcoin#26355 (comment) ACKs for top commit: hernanmarino: ACK 784b023 brunoerg: crACK 784b023 mzumsande: ACK 784b023 Tree-SHA512: b47ac0d78a09ca3a1806e38c5d2e2fcf1e5f0668f202450b5079c5cb168e168ac6828c0948d23f3610696375134986d75ef3c6098858173023bcb743aec8004c Co-authored-by: fanquake <fanquake@gmail.com>
…_minchainwork fa952fa test: Avoid rpc timeout in p2p_headers_sync_with_minchainwork (MarcoFalke) Pull request description: When running a lot of tests in parallel, I get `JSONRPCException: 'generatetoaddress' RPC took longer than 30.000000 seconds.` The general recommendation, if running into timeouts, is to increase the `--timeout-factor`. However, I think that the default timeout values should be suitable to run the tests out of the box on reasonable hardware. ACKs for top commit: fanquake: ACK fa952fa Tree-SHA512: b7eeda54f8db900f077417c0431f659c67e686e2fc078f8c713e37ed75b8bc862814ce20e8400741638e35e224d7284ad16172bf5f82168f803376d0c9ec4524 Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
…id proof-of-work disconnects peer 7726712 test: p2p: check that headers message with invalid proof-of-work disconnects peer (Sebastian Falbesoner) Pull request description: One of the earliest anti-DoS checks done after receiving and deserializing a `headers` message from a peer is verifying whether the proof-of-work is valid (called in method `PeerManagerImpl::ProcessHeadersMessage`): https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2752-L2762 The called method `PeerManagerImpl::CheckHeadersPoW` calls `Misbehaving` with a score of 100, i.e. leading to an immediate disconnect of the peer: https://github.com/bitcoin/bitcoin/blob/f227e153e80c8c50c30d76e1ac638d7206c7ff61/src/net_processing.cpp#L2368-L2372 This PR adds a simple test for both the misbehaving log and the resulting disconnect. For creating a block header with invalid proof-of-work, we first create one that is accepted by the node (the difficulty field `nBits` is copied from the genesis block) and based on that the nonce is modified until we have block header hash prefix that is too high to fulfill even the minimum difficulty. ACKs for top commit: Sjors: ACK 7726712 achow101: ACK 7726712 brunoerg: crACK 7726712 furszy: Code review ACK 7726712 with a non-blocking speedup. Tree-SHA512: 680aa7939158d1dc672b90aa6554ba2b3a92584b6d3bcb0227776035858429feb8bc66eed18b47de0fe56df7d9b3ddaee231aaeaa360136603b9ad4b19e6ac11 Co-authored-by: Andrew Chow <github@achow101.com>
…sync_with_minchainwork.py fa247e6 test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py (MarcoFalke) Pull request description: Similar to bitcoin#30705: The goal of this test case is to check that the sync works at all, not to check any timeout. On extremely slow hardware (for example qemu virtual hardware), downloading the 4110 BLOCKS_TO_MINE may take longer than the block download timeout. Fix it by pinning the time using mocktime temporarily, and advance it immediately after the sync. ACKs for top commit: stratospher: ACK fa247e6. Checked the timeout downloading block logs before/after using `setmocktime`. tdb3: ACK fa247e6 Tree-SHA512: f61632a8d9e484f1b888aafbf87f7adf71b8692387bd77f603cdbc0de49f30d42e654741d46ae1ff8b9706a5559ee0faabdb192ed0db7449010b68bfcdbaa42d
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Issue being fixed or feature implemented
The backports of pre-sync feature introduced multiple hash-recalculations during header pre-sync which potentially makes header sync stage slower, see #7318
What was done?
This PR applies optimization from #7272 to pre-sync feature
How Has This Been Tested?
TBD bench testnet / mainnet 7318, pre-7318 and this PR
This PR is draft until 7318 is merged
Breaking Changes
N/A
Checklist: