coins: cache CCoinsMap outpoint hashes#162
Draft
l0rinc wants to merge 15 commits into
Draft
Conversation
send_batch_request() was building raw dicts without a "jsonrpc" version, which made Core to treat them as version 1.0 requests. This worked in normal mode, but failed with --usecli because TestNodeCLI.batch() expects callables, not dicts. This commit fixes it by using get_request() which is defined in both AuthServiceProxy and TestNodeCLIAttr. The assert is changed because by using get_reques() AuthServiceProxy treats it as "jsonrpc" version 2.0 requests, which don't return "error" keys.
Add the flag --extended to a test (00_setup_env_i686_no_ipc.sh) with the --usecli flag to cover all tests with --usecli.
The `coinscache_sim` fuzz target builds a 23-byte P2SH scriptPubKey manually. Place `OP_EQUAL` at index 22, after `OP_HASH160`, the 20-byte push opcode, and the 20-byte script hash. This matches `CScript::IsPayToScriptHash()`, which checks byte 22 for `OP_EQUAL`, see src/script/script.cpp#L229
Add a `CTestConfig.cmake` file with the CDash submission URL for Bitcoin Core. This lets developers use CTest’s built-in dashboard support to upload local configure, build, and test results to CDash. With this a developer can use their own CTest dashboard script or manual `ctest` workflow and upload results to a collection point. This is useful for sharing reproducible build/test results from local machines, CI experiments, and platform-specific debugging. CDash keeps the configure output, build warnings/errors, and test results grouped under a single dashboard submission, making it easier to inspect failures and compare behavior across environments. The file only defines the CDash upload location. It does not change the default build or test behavior.
…egral casts (take 3) fa864b9 refactor: [rpc] Remove confusing and brittle integral casts (take 3) (MarcoFalke) Pull request description: This one cast is harmless, but confusing. Also, in the past they have been brittle to the extend of triggering bugs. See commit 44afed4. So remove this one recently introduced one. ACKs for top commit: fjahr: ACK fa864b9 stickies-v: ACK fa864b9 sedited: ACK fa864b9 Tree-SHA512: 78407b97964c144f4d94cd445af4f57e29e460912ae9178898224d71f3d5237e5db88a981f7636193a6b7a22be8ad4fd833eb0efa9507284c10f87d9da0ec81b
ac58e6c test: fix P2SH output in coins cache fuzz (Lőrinc) Pull request description: ### Problem `coinscache_sim` manually constructs a 23-byte P2SH `scriptPubKey`, but placed `OP_EQUAL` at byte index 12. That index is inside the 20-byte script hash payload, so the constructed script did not match the standard P2SH layout: https://github.com/bitcoin/bitcoin/blob/fa2670bd4b5b199c417011942228ba87d1613030/src/script/script.cpp#L223-L230 ### Fix Place `OP_EQUAL` after `OP_HASH160`, the 20-byte push opcode, and the 20-byte script hash. Also remove a stray trailing comment terminator in the same fuzz target. ACKs for top commit: Crypt-iQ: crACK ac58e6c brunoerg: ACK ac58e6c sedited: ACK ac58e6c Tree-SHA512: 1f909dd25d9df87a923e6496145da0ada2b1fa6511b61fb2d203db4c7724f2341c898862a15e7051b952bca834e6654c70fba64a7bf223bfd6d399b3b5d9e59b
…--usecli a49bc1e ci: add --extended when using --usecli (Pol Espinasa) 5603ae0 test: fix send_batch_request to pass callables when using --usecli (Pol Espinasa) Pull request description: **First commit** This commit fixes an error that made feature_index_prune.py fail if --usecli was used. The test was failing because `node.batch(data)` was called with `data` being a dict. This worked in the normal scenario because `AuthServiceProxy.batch()` expects a list of petitions in the form of a dict. But `TestNodeCLI.batch()` expects callables and not a dict. When `--usecli` is used the test fails with an error `TypeError: 'dict' object is not callable`. This is fixed by using `get_request()` which returns a lambda function if `--usecli` is used and returns a dict if not. The `assert` is also changed because before this PR, the requests, constructed by hand were not specifiying the json-rpc version. By default if no version is specified we use version 1.0 which always returns `error: none` if there is no error. However, in version 2.0, it does not include an error key if there is no error. By using `get_request()` the requests are done in version 2.0 so there's no key error. **Second commit** The current CI doesn't cover the case of running all tests with --extended if using --usecli. This led to a test failing (feature_index_prune.py) if run with --usecli and the CI not catching it. See bitcoin#34991 for context. This commit improves the CI test coverage by also running now all functional tests (--extended) with the flag --usecli. <details> <summary>First "wrong" approach</summary> Fixes two bugs that make `feature_index_prune.py` fail if `--usecli` is used. 1. Makes `TestNodeCLI.batch()` response equivalent to what a JSON-RPC response would look like by adding `error=None` in the response. The lack of `error` in the response was giving a `KeyError` message. 2. Makes `send_batch_request()` compatible with --usecli. Before the PR it was passing dicts to `node.batch()`, but `TestNodeCLI.batch()` expects callables, not dicts. </details> ACKs for top commit: Bicaru20: Re-ACK a49bc1e. sedited: Re-ACK a49bc1e Tree-SHA512: 75fca26cf120638ced1fe38e86d8e3efa7addb6d97fc801e34783efd2cf6417f4ded2ec6247b6dcbcdb3cf4f48c4858f0932cbaa3e836a973d53581e75470a3f
0651a1f doc: Add Niklas Goegge's key to SECURITY.md (dergoegge) Pull request description: Same as bitcoin-core/bitcoincore.org#1244 ACKs for top commit: achow101: ACK 0651a1f stickies-v: ACK 0651a1f sedited: ACK 0651a1f Tree-SHA512: 1ded3b9c7e2c2a8cecef8d1b4ccb7caac4d8162f5b83e2e02a0e7367f3f2bac688c9672e6d02e631fa55e07e34f55af05743e84323150e959c661d2bf9ea1a10
…lpine task fad6189 ci: Move --usecli --extended from i386 task to alpine task (MarcoFalke) Pull request description: The i386 task is getting increasingly tedious to maintain: * It is using deprecated (disabled by default) syscalls, see commit 999e9db * Running it on (e.g.) aarch64 is slow, due to the additional qemu overhead It is mostly kept to have some more 32-bit coverage for bitcoin#32375. But maybe in 5 or 10 years, it can be removed .... ? So for now, try to reduce the config it runs by moving it out to longer-term tasks. Specifically, move `TEST_RUNNER_EXTRA="--v2transport --usecli --extended"` to the native Alpine task, which should exist long-term, runs natively, and also has a debug build enabled. ACKs for top commit: polespinasa: ACK fad6189 sedited: ACK fad6189 Tree-SHA512: a406347aa06de7edbfe8c7d3d082983fa54f5c266c1b2344102cd6cd59dc5b9f46da5e15edfaa7655edf1a2ff8d70da18bae4c8297dfb0cf2c783098d8a46c30
0868940 cmake: add CTestConfig.cmake (will) Pull request description: Add a `CTestConfig.cmake` file with the CDash submission URL for Bitcoin Core at the project root. This lets developers use CTest’s built-in dashboard support to upload local configure, build, and test results to CDash. With this a developer can use their own CTest dashboard script or manual `ctest` workflow and upload results to a collection point. This is useful for sharing reproducible build/test results from local machines, CI experiments, and platform-specific debugging. CDash keeps the configure output, build warnings/errors, and test results grouped under a single dashboard submission, making it easier to inspect failures and compare behavior across environments. The file only defines the CDash upload location. It does not change the default build or test behavior. **WARNING** uploaded test results may also include (potentially) identifiable information, such as hostnames, usernames, especially if you attach notes which are located within you rhome directory, as the full file path is printed. ACKs for top commit: purpleKarrot: ACK 0868940 maflcko: lgtm ACK 0868940 hebasto: ACK 0868940. Tree-SHA512: 5d0e2e174773de3f56af80252a62330fc313f394e1a868ac0d2a83c6610c2f734f6e6938bfced710879c00f64f09e17030da594b24940d77c984e46698d37647
PoolAllocator's node budget should only reserve space for implementation-defined std::unordered_map node-link overhead before the key caches any hash value. Drop the stale wording about standard-library hash-code storage and use 3 * sizeof(void*) consistently across the PoolAllocator unordered-map examples.
Keep SaltedOutpointHasher noexcept so normal unordered containers do not ask libstdc++ to store hash codes in every node. CCoinsMap still benefits from avoiding repeated salted outpoint hashing. Add a CCoinsMap-specific hasher that caches the hash in COutPoint and fills it through std::atomic_ref, so concurrent lookups can race only by recomputing and publishing the same cached value. Bump the CCoinsMap PoolAllocator slack back to four pointer-sized fields. CCoinsMap is the dominant memory consumer, so keep a conservative allowance for implementation-defined std::unordered_map node overhead instead of risking fall-through allocations on stdlibs with larger nodes.
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
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.
Keep SaltedOutpointHasher noexcept so normal unordered containers do not ask libstdc++ to store hash codes in every node.
CCoinsMap still benefits from avoiding repeated salted outpoint hashing. Cache the hash in COutPoint and let CCoinsMap use a map-specific hasher id so copied cache entries do not reuse a hash from a different map salt.
The cached hash is now part of the key object itself, so restore the pool allocator budget to four pointer-sized fields.