backport: merge bitcoin#21727, #22371, #21526, #23174, #23785, #23581, #23974, #22932, #24050, #24515 (blockstorage backports)#6085
Conversation
96904b0 to
4e45b85
Compare
|
Sanitizer tsan crashes like that in CI: I guess, this crash happens due to re-creation of g_txindex inside so, g_txindex can be re-created several times, but there are async calls of g_txindex and they can be using g_txindex, when it called to re-create it. I guess some guard code is needed for it. |
…cation and into constructor 77915de test: run `Interrupt()` before `Stop()`, add additional sanity check (Kittywhiskers Van Gogh) a376e93 fix: init `g_txindex` only once, downgrade from assert to exception (Kittywhiskers Van Gogh) Pull request description: ## Motivation `g_txindex` should be initialized in `TestChainSetup`'s constructor but when backporting [bitcoin#19806](6bf39d7#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R289) ([dash#5236](#5236)), portions of the constructor were split into `TestChainSetup::mineBlocks()`, `g_txindex`'s init was left behind in the latter instead of the former. This meant that every `mineBlocks()` call would re-create a `TxIndex` instance, which is not intended behaviour; and was recorded to cause `heap-use-after-free`s ([comment](#6085 (comment)), also the reason this PR was opened). This PR aims to resolve that. ## Additional Information * Crashes stemming from previous attempts (except for one attempt) were not reproducible with my regular local setup (`depends` built with Clang 16, Dash Core built with Clang 16, set of debug-oriented flags, unit tests run using `./src/test/test_dash`). * Attempting to rebuild Dash Core with GCC 9 was insufficient, required to rebuild depends with GCC 9 as well * `configure`'d with `CC=gcc CXX=g++ CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20 --disable-ccache` * Unit tests must be run with `make check-recursive -j$(( $(nproc --all) - 2 ))` * An index must be initialized **after** the chain is constructed, this seems to be corroborated by all other index usage ([source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/blockfilter_index_tests.cpp#L141), [source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/coinstatsindex_tests.cpp#L33), [source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/txindex_tests.cpp#L31), all three use `Start()` for their respective indexes _after_ `TestChain100Setup`'s constructor runs `mineBlocks()`) * Attempting to run `Start()` earlier (_before_ the `mineBlocks()` call in the constructor) results in erratic behaviour * This also explains why my attempt at moving it back to `TestingSetup` (a grandparent of `TestChainSetup`) failed * `Interrupt()` is supposed to be called before `Stop()` but this was erroneously removed in a [commit](cc9dcdd#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L413-L419) that adopted `IndexWaitSynced`. This has since been resolved. * In line [with](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/blockfilter_index_tests.cpp#L138-L139) [other](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/coinstatsindex_tests.cpp#L29-L31) [indexes](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/txindex_tests.cpp#L28-L29), an sanity check has been added. Additionally, as `TxIndex::Start()` is more akin to `CChainState::LoadGenesisBlock()` than `CChainState::CanFlushToDisk()`, the `assert` has been downgraded to an exception. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 77915de UdjinM6: utACK 77915de PastaPastaPasta: utACK 77915de Tree-SHA512: 5051dcf62b6cad4597044b4d27dc54c5bafa8692ba31e03c8afc0f7ef80b790a3873cf0275bb2cf6260939a11f95625da79fc7987951e66114900a86276c037c
Bitcoin uses underscore in util/translation.h for translatable strings but underscores are also a placeholder used in multiple languages, incl. C++. The next commit will be introducing backports, one of them will be placing util/translation.h into validation.h, which is a header used by Dash-specific code. This causes a conflict between the normal usage of underscore as a placeholder and Bitcoin's usage of it as a function name, which is reported by the Dash-specific linter. We need to exclude shadowing warnings from the linter to account for this.
…nFile/nDataPos/nUndoPos by cs_main
| return BlockFileSeq().FileName(pos); | ||
| } | ||
|
|
||
| // TODO move to blockstorage |
There was a problem hiding this comment.
21727: nit: should drop this line I guess
knst
left a comment
There was a problem hiding this comment.
utACK 1bf0bf4
see nits if you will rebase it
+consider take these backports as useful follow-ups: bitcoin#23154 , bitcoin#24002, maybe new PR?
also maybe 24197
| BlockValidationState state; | ||
| fCheckForPruning = true; | ||
|
|
||
| m_blockman.m_check_for_pruning = true; |
There was a problem hiding this comment.
nit: should not remove this empty line
| @@ -93,25 +93,6 @@ const std::vector<std::string> CHECKLEVEL_DOC { | |||
| "each level includes the checks of the previous levels", | |||
There was a problem hiding this comment.
nit: (24515) have missing changes due to no namespace node::
, bitcoin#23522, bitcoin#24026, bitcoin#24104, bitcoin#24167, bitcoin#20744, partial bitcoin#23469, bitcoin#24169 (replace boost::filesystem with std::filesystem) 0f23920 partial bitcoin#24169: Add --enable-c++20 option (Kittywhiskers Van Gogh) a3b7926 merge bitcoin#20744: Use std::filesystem. Remove Boost Filesystem & System (Kittywhiskers Van Gogh) be7ac49 merge bitcoin#24167: consistently use fsbridge:: for ifstream / ofstream (Kittywhiskers Van Gogh) 7ffea43 merge bitcoin#24104: Make compatible with boost 1.78 (Kittywhiskers Van Gogh) 7c270e6 merge bitcoin#24026: Block unsafe std::string fs::path conversion copy_file calls (Kittywhiskers Van Gogh) b0d2484 merge bitcoin#23522: Improve fs::PathToString documentation (Kittywhiskers Van Gogh) 20d359b partial bitcoin#23469: Remove Boost build note from build-unix.md (Kittywhiskers Van Gogh) 193f6fd merge bitcoin#23446: Mention that BerkeleyDB is for legacy wallet in build-unix (Kittywhiskers Van Gogh) ecfac10 merge bitcoin#22937: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method (Kittywhiskers Van Gogh) 23fe7e2 chore: dashify symbols in some unit tests (Kittywhiskers Van Gogh) 28b96a0 merge bitcoin#22840: fix unoptimized libraries in depends (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6085 * Depends on #6137 * Dependency for #6150 ## Breaking Changes None observed. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 0f23920 PastaPastaPasta: utACK 0f23920 Tree-SHA512: 4b8f0ae55185ece27d8084a5339196b7ed993c8138f4c59a0db3e16729d4edf4a59a68a8c7309c32df57734c07182821c4878b55c253da5df763204ad7158426
, bitcoin#24178, bitcoin#24171, bitcoin#25404, bitcoin#25514, bitcoin#25720, partial bitcoin#23832, bitcoin#24169, bitcoin#25454 (headers backports) c92b0f5 merge bitcoin#25720: Reduce bandwidth during initial headers sync when a block is found (Kittywhiskers Van Gogh) 0f9ece0 merge bitcoin#25514: Move CNode::nServices and CNode::nLocalServices to Peer (Kittywhiskers Van Gogh) c9923ca partial bitcoin#25454: Avoid multiple getheaders messages in flight to the same peer (Kittywhiskers Van Gogh) 26d477b revert: Fix duplicate initial headers sync (Kittywhiskers Van Gogh) abccb2d test: drop genesis block from `blockheader_testnet3` (Kittywhiskers Van Gogh) 0574a7d merge bitcoin#25404: Use MAX_BLOCKS_TO_ANNOUNCE consistently (Kittywhiskers Van Gogh) ed871d2 merge bitcoin#24171: Sync chain more readily from inbound peers during IBD (Kittywhiskers Van Gogh) a04290f merge bitcoin#24178: Respond to getheaders if we have sufficient chainwork (Kittywhiskers Van Gogh) bcafa28 merge bitcoin#24909: Move and rename pindexBestHeader, fHavePruned (Kittywhiskers Van Gogh) 70485cb partial bitcoin#24169: Add --enable-c++20 option (Kittywhiskers Van Gogh) 27e885d merge bitcoin#23880: Serialize cmpctblock at most once in NewPoWValidBlock (Kittywhiskers Van Gogh) 9f7ac69 merge bitcoin#24024: Remove cs_main lock annotation from ChainstateManager.m_blockman (Kittywhiskers Van Gogh) 9399f90 partial bitcoin#23832: Changes time variables from int to chrono (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6085 * Dependency for #6098 * ~~[bitcoin#25514](bitcoin#25514) removes `peers.spvNodeConnections` and `peers.fullNodeConnections` reporting from `CalculateNumConnectionsChangedStats` as the services flags used to distingush between the two have been moved to the `Peer` struct, accessable only through `PeerManager`.~~ ~~As `PeerManager` isn't accessable to `CConnman`, even if a new public function was exposed through `PeerManger` (as we have for `IsInvInFilter` and others or we try to access the value through `GetNodeStateStats`), `CConnman` would be unable to leverage it.~~ Resolved with patch by UdjinM6, thanks! * [bitcoin#23880](bitcoin#23880) introduces code that is not valid C++20 (but valid C++17) and therefore, required a partial backport of [bitcoin#24169](bitcoin#24169) (fae6790) to make the code C++20 legal. * [bitcoin#25454](bitcoin#25454) introduces a 10-point penalty for remitting more than `MAX_BLOCKS_TO_ANNOUNCE` unconnected block headers. `blockheader_testnet3.hex` (introduced in [bitcoin#16551](bitcoin#16551), part of [dash#5963](#5963)), unlike in Bitcoin, includes the genesis block. By definition of a genesis block, there is no block before it that connects to, which causes the 10-point penalty to trip and `p2p_dos_header_tree.py` to fail (see below). This has been remedied by removing the genesis block from the test data to match upstream and also because no node has a good reason to ever broadcast the genesis block as-is over P2P. <details> <summary>Test Failure</summary> ``` dash@6a2649cc721f:/src/dash$ ./test/functional/p2p_dos_header_tree.py 2024-06-17T17:59:35.874000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_h5hfluy1 2024-06-17T17:59:36.892000Z TestFramework (INFO): Read headers data 2024-06-17T17:59:36.895000Z TestFramework (INFO): Feed all non-fork headers, including and up to the first checkpoint 2024-06-17T17:59:38.411000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 158, in main self.run_test() File "./test/functional/p2p_dos_header_tree.py", line 53, in run_test assert { AssertionError 2024-06-17T17:59:38.913000Z TestFramework (INFO): Stopping nodes 2024-06-17T17:59:39.917000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_h5hfluy1 2024-06-17T17:59:39.917000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_h5hfluy1/test_framework.log 2024-06-17T17:59:39.917000Z TestFramework (ERROR): 2024-06-17T17:59:39.917000Z TestFramework (ERROR): Hint: Call /src/dash/test/functional/combine_logs.py '/tmp/dash_func_test_h5hfluy1' to consolidate all logs 2024-06-17T17:59:39.917000Z TestFramework (ERROR): 2024-06-17T17:59:39.917000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2024-06-17T17:59:39.917000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues 2024-06-17T17:59:39.917000Z TestFramework (ERROR): ``` </details> * [bitcoin#25454](bitcoin#25454) has a goal similar to [dash#2032](#2032) (and its predecessor, [dash#1589](#1589)), namely, avoiding `getheaders`(`2`) duplication to the same peer. Unfortunately, Dash's mitigation seems to conflict with Bitcoin's mitigation and this results in `feature_minchainwork.py` failing (see below). This has been remedied by partially reverting [dash#2032](#2032). <details> <summary>Test Failure</summary> ``` dash@1bebec413839:/src/dash$ ./test/functional/feature_minchainwork.py 2024-08-01T17:29:41.116000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_co8xkx43 2024-08-01T17:29:42.145000Z TestFramework (INFO): Testing relay across node 1 (minChainWork = 101) 2024-08-01T17:29:42.145000Z TestFramework (INFO): Generating 49 blocks on node0 [...] 2024-08-01T17:29:51.707000Z TestFramework (INFO): Generating one more block 2024-08-01T17:29:51.709000Z TestFramework (INFO): Verifying nodes are all synced 2024-08-01T17:30:51.989000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 159, in main self.run_test() File "./test/functional/feature_minchainwork.py", line 101, in run_test self.sync_all() File "/src/dash/test/functional/test_framework/test_framework.py", line 811, in sync_all self.sync_blocks(nodes) File "/src/dash/test/functional/test_framework/test_framework.py", line 777, in sync_blocks raise AssertionError("Block sync timed out after {}s:{}".format( AssertionError: Block sync timed out after 60s: '00ab061e30aebd2f97153d26cd72f921af896f05c6469ad73c7de4fc283d9590' '00ab061e30aebd2f97153d26cd72f921af896f05c6469ad73c7de4fc283d9590' '000008ca1832a4baf228eb1553c03d3a2c8e02399550dd6ea8d65cec3ef23d2e' 2024-08-01T17:30:52.490000Z TestFramework (INFO): Stopping nodes 2024-08-01T17:30:53.495000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_co8xkx43 2024-08-01T17:30:53.495000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_co8xkx43/test_framework.log 2024-08-01T17:30:53.495000Z TestFramework (ERROR): 2024-08-01T17:30:53.495000Z TestFramework (ERROR): Hint: Call /src/dash/test/functional/combine_logs.py '/tmp/dash_func_test_co8xkx43' to consolidate all logs 2024-08-01T17:30:53.495000Z TestFramework (ERROR): 2024-08-01T17:30:53.495000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2024-08-01T17:30:53.495000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues 2024-08-01T17:30:53.495000Z TestFramework (ERROR): ``` </details> * [bitcoin#25720](bitcoin#25720) introduces a new test, `p2p_initial_headers_sync.py`, to validate that when a client has a stale tip, it will only engage in headers sync with one peer (emit a `getheaders2`\* message). Unmodified, this test fails (see below) as while the backport deals with one source of `getheaders2` messages, the test setup unwittingly triggers another ([source](https://github.com/dashpay/dash/blob/2379462294da884d925b5a80acec20fbc360309f/src/net_processing.cpp#L5446-L5448)), specifically, allowing the `pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge` condition to evaluate `true`. This is because, unlike in Bitcoin test suite's `setup_chain()` ([source](https://github.com/bitcoin/bitcoin/blob/22d96d76ab02fc73e7fe0d810bacee4c982df085/test/functional/test_framework/test_framework.py#L379-L385)), Dash sets the mocktime to match the mock chain ([source](https://github.com/dashpay/dash/blob/2379462294da884d925b5a80acec20fbc360309f/test/functional/test_framework/test_framework.py#L408-L416)) during setup, while the test assumes that the mock chain is stale enough to not trigger this source of `getheaders2` messages. As the tip is barely stale, it emits the `getheaders2` message, which is detected, causing the test to fail. This has been remedied by overriding `setup_chain()` to behave more like Bitcoin's test suite. _\* - `getheaders2` is a Dash-specific message that is courtesy of compressed headers, Bitcoin would be checking for `getheaders`_ <details> <summary>Test Failure</summary> ``` dash@6a2649cc721f:/src/dash$ ./test/functional/p2p_initial_headers_sync.py 2024-06-17T21:24:09.921000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_3gypo3ab 2024-06-17T21:24:10.681000Z TestFramework (INFO): Adding a peer to node0 2024-06-17T21:24:11.684000Z TestFramework (INFO): Connecting two more peers to node0 2024-06-17T21:24:13.689000Z TestFramework (INFO): Verify that peer2 and peer3 don't receive a getheaders after connecting 2024-06-17T21:24:15.193000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 158, in main self.run_test() File "./test/functional/p2p_initial_headers_sync.py", line 60, in run_test assert "getheaders2" not in peer2.last_message AssertionError 2024-06-17T21:24:15.695000Z TestFramework (INFO): Stopping nodes 2024-06-17T21:24:16.698000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_3gypo3ab 2024-06-17T21:24:16.698000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_3gypo3ab/test_framework.log 2024-06-17T21:24:16.699000Z TestFramework (ERROR): 2024-06-17T21:24:16.699000Z TestFramework (ERROR): Hint: Call /src/dash/test/functional/combine_logs.py '/tmp/dash_func_test_3gypo3ab' to consolidate all logs 2024-06-17T21:24:16.699000Z TestFramework (ERROR): 2024-06-17T21:24:16.699000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2024-06-17T21:24:16.699000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues 2024-06-17T21:24:16.699000Z TestFramework (ERROR): ``` </details> ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 16b7c42195e197e8b6800c3425b4ff15a124b0e3e0da5c98ca6e22486b52c4ea82f2f05b83e28e838860b1ce76daa1e435c5eae4fb7591a161f26a5fff6189cc
Additional Information
Dependent on refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) #6078
Dependent on backport: merge bitcoin#18344, #20867, #20286, #21359, #21910, #19651, #21934, #22722, #20295, #23702, partial bitcoin#23706 (rpc backports) #6074
Dependent on refactor: move Get*Index to rpc/index_util.cpp, const-ify functions and arguments, add lock annotations and some minor housekeeping #6083
Dependent on fix: move
g_txindexinitialization out of erroneous location and into constructor #6119Dependency for backport: merge bitcoin#22840, #22937, #23446, #23522, #24026, #24104, #24167, #20744, partial bitcoin#23469, #24169 (replace boost::filesystem with std::filesystem) #6138
In bitcoin#24050,
BlockMapis given ownership of theCBlockIndexinstance contained within theunordered_map. The same has not been done forPrevBlockMapasPrevBlockMapis populated withpprevpointers and doing so seems to break validation logic.Dash has a specific linter for all Dash-specific code present in Core. The introduction of
util/translation.hintovalidation.hhas caused the linter to trigger shadowing warnings due to a conflict between the common use of_as a placeholder/throwaway name (source) and upstream's usage of it to process translatable strings (source).Neither C++17 nor C++20 have an official placeholder/throwaway term or annotation for structured bindings (which cannot use
[[maybe_unused]orstd::ignore) but P2169 is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore.Breaking Changes
None expected
Checklist: