backport: bitcoin#24322, #25064, #25065, #25168, #25281, #25290, #25307, #25487, #25607 (kernel backports)#7361
Open
knst wants to merge 11 commits into
Open
backport: bitcoin#24322, #25064, #25065, #25168, #25281, #25290, #25307, #25487, #25607 (kernel backports)#7361knst wants to merge 11 commits into
knst wants to merge 11 commits into
Conversation
|
✅ Review complete (commit 4614356) |
knst
changed the title
WalkthroughThis PR adds kernel context, kernel sanity checks, and mempool persistence APIs. It refactors mempool, chainstate, and validation code to use kernel-backed options, explicit paths, and adjusted-time callbacks. It also updates node startup, peer management, mining, RPC, tests, build wiring, and lint/functional test inputs to match the new interfaces. Sequence Diagram(s)sequenceDiagram
participant Init as init.cpp
participant NodeContext as node::NodeContext
participant KernelContext as kernel::Context
participant ChainstateManager as ChainstateManager
participant CTxMemPool as CTxMemPool
participant MempoolPersist as kernel::DumpMempool
Init->>NodeContext: create kernel context and mempool options
NodeContext->>KernelContext: construct
Init->>ChainstateManager: construct with ChainstateManager::Options
Init->>CTxMemPool: construct with MemPoolOptions
Init->>MempoolPersist: DumpMempool(mempool, MempoolPath(args))
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/rawtransaction.cpp (1)
379-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete migration from global
Params()tochainmanparameters across RPC endpoints. The refactoring updatesgetrawtransaction()to use the active chainman's consensus parameters instead of the global singleton, but similar operations ingetrawtransactionmulti()andgettxchainlocks()still use the globalParams(). This creates inconsistent consensus parameter sourcing across functionally identical RPC endpoints and conflicts with the PR's stated goal to decouple from global singletons.
src/rpc/rawtransaction.cpp#L379-L407: correctly migrated tochainman.GetParams()andchainman.GetConsensus().src/rpc/rawtransaction.cpp#L524-L524: replaceParams().GetConsensus()withchainman.GetConsensus()in theGetTransactioncall withingetrawtransactionmulti().src/rpc/rawtransaction.cpp#L670-L670: replaceParams().GenesisBlock().hashMerkleRootwithchainman.GetParams().GenesisBlock().hashMerkleRootin the genesis check withingettxchainlocks().src/rpc/rawtransaction.cpp#L679-L679: replaceParams().GetConsensus()withchainman.GetConsensus()in theGetTransactioncall withingettxchainlocks().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/rawtransaction.cpp` around lines 379 - 407, The code mixes global Params()/GetConsensus() with chainman; unify to use chainman as done in the anchor region: in src/rpc/rawtransaction.cpp (lines 379-407) the pattern is correct; for src/rpc/rawtransaction.cpp (line 524) inside getrawtransactionmulti() replace Params().GetConsensus() with chainman.GetConsensus() in the GetTransaction call; for src/rpc/rawtransaction.cpp (line 670) inside gettxchainlocks() replace Params().GenesisBlock().hashMerkleRoot with chainman.GetParams().GenesisBlock().hashMerkleRoot for the genesis coinbase check; and for src/rpc/rawtransaction.cpp (line 679) inside gettxchainlocks() replace Params().GetConsensus() with chainman.GetConsensus() in the GetTransaction call so all RPCs consistently use chainman for params/consensus (adjust include/use of chainman variable if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configure.ac`:
- Around line 1800-1804: The AC_MSG_ERROR currently triggers whenever
build_experimental_kernel_lib = "no" even if chainstate isn't requested; change
the logic so the error is emitted only when both build_bitcoin_chainstate =
"yes" and build_experimental_kernel_lib = "no". Concretely, replace the
top-level if that checks build_experimental_kernel_lib with a conditional that
tests both variables (e.g., if test "$build_bitcoin_chainstate" = "yes" && test
"$build_experimental_kernel_lib" = "no"; then AC_MSG_ERROR(...); else
AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate =
"yes"]) fi), leaving the AM_CONDITIONAL for BUILD_BITCOIN_CHAINSTATE unchanged.
In `@src/kernel/checks.h`:
- Around line 14-19: The enum value ERROR_CHRONO is unreachable because
kernel::SanityChecks() never returns it while AppInitSanityChecks still handles
it; fix by either implementing the missing chrono sanity check in
kernel::SanityChecks() so it can return SanityCheckError::ERROR_CHRONO (add a
chrono validity test and return that enum on failure), or remove ERROR_CHRONO
from the SanityCheckError enum and delete the corresponding case in
AppInitSanityChecks (and update any switch/default logic). Ensure the chosen
change keeps enum and switch consistent and compile-clean by updating any
related tests/comments referencing ERROR_CHRONO.
In `@src/node/chainstate.cpp`:
- Line 251: The comparison in VerifyLoadedChainstate uses GetTime() directly,
bypassing the injected time callback; replace the hardcoded GetTime() call with
the provided get_unix_time_seconds() callback so the check if (tip && tip->nTime
> ...) becomes consistent with the injected time source (update the code in
VerifyLoadedChainstate where tip->nTime is compared to GetTime() +
MAX_FUTURE_BLOCK_TIME to use get_unix_time_seconds() instead).
In `@src/test/fuzz/tx_pool.cpp`:
- Around line 120-131: The code currently constructs a CTxMemPool via
MakeMempool but then UB-ly downcasts tx_pool_ to MockedTxPool with
*static_cast<MockedTxPool*>(&tx_pool_); fix this by actually constructing a
MockedTxPool instead of a CTxMemPool where the cast is used: either change the
declaration tx_pool_ to be MockedTxPool (and update MakeMempool to return a
MockedTxPool or add a MakeMockedMempool helper that builds the same Options and
constructs a MockedTxPool), and remove the static_cast usages; do not leave a
CTxMemPool and cast it to MockedTxPool. Ensure references to CTxMemPool,
MockedTxPool, tx_pool_, MakeMempool and the static_cast sites are updated
consistently.
In `@src/test/mempool_tests.cpp`:
- Around line 20-24: The test currently static_casts m_node.mempool to
MemPoolTest&, which is UB because m_node.mempool is actually a
std::unique_ptr<CTxMemPool>; fix by constructing a real MemPoolTest object
instead of a CTxMemPool: replace the
std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node)) allocation with
std::make_unique<MemPoolTest>(MemPoolOptionsForTest(m_node)) so the dynamic type
matches MemPoolTest, or alternatively change the fixture/member type that holds
m_node.mempool from std::unique_ptr<CTxMemPool> to std::unique_ptr<MemPoolTest>
and initialize it with MemPoolOptionsForTest(m_node); ensure all usages that
rely on CTxMemPool still compile against the MemPoolTest subclass.
---
Outside diff comments:
In `@src/rpc/rawtransaction.cpp`:
- Around line 379-407: The code mixes global Params()/GetConsensus() with
chainman; unify to use chainman as done in the anchor region: in
src/rpc/rawtransaction.cpp (lines 379-407) the pattern is correct; for
src/rpc/rawtransaction.cpp (line 524) inside getrawtransactionmulti() replace
Params().GetConsensus() with chainman.GetConsensus() in the GetTransaction call;
for src/rpc/rawtransaction.cpp (line 670) inside gettxchainlocks() replace
Params().GenesisBlock().hashMerkleRoot with
chainman.GetParams().GenesisBlock().hashMerkleRoot for the genesis coinbase
check; and for src/rpc/rawtransaction.cpp (line 679) inside gettxchainlocks()
replace Params().GetConsensus() with chainman.GetConsensus() in the
GetTransaction call so all RPCs consistently use chainman for params/consensus
(adjust include/use of chainman variable if needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e36e1775-c57a-44f6-ab45-54cb8d77291c
📒 Files selected for processing (87)
ci/dash/lint-tidy.shci/test/00_setup_env_native_nowallet_libbitcoinkernel.shconfigure.accontrib/devtools/iwyu/bitcoin.core.impsrc/Makefile.amsrc/Makefile.test_fuzz.includesrc/bitcoin-chainstate.cppsrc/bitcoind.cppsrc/bls/bls.cppsrc/bls/bls.hsrc/deploymentstatus.hsrc/evo/mnhftx.cppsrc/fs.hsrc/init.cppsrc/init.hsrc/init/common.cppsrc/init/common.hsrc/kernel/bitcoinkernel.cppsrc/kernel/chainstatemanager_opts.hsrc/kernel/checks.cppsrc/kernel/checks.hsrc/kernel/context.cppsrc/kernel/context.hsrc/kernel/mempool_limits.hsrc/kernel/mempool_options.hsrc/kernel/mempool_persist.cppsrc/kernel/mempool_persist.hsrc/mempool_args.cppsrc/mempool_args.hsrc/net_processing.cppsrc/net_processing.hsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/context.cppsrc/node/context.hsrc/node/interfaces.cppsrc/node/mempool_persist_args.cppsrc/node/mempool_persist_args.hsrc/node/miner.cppsrc/node/miner.hsrc/policy/fees.cppsrc/policy/fees.hsrc/policy/fees_args.cppsrc/policy/fees_args.hsrc/policy/packages.hsrc/policy/policy.hsrc/rest.cppsrc/rpc/blockchain.cppsrc/rpc/fees.cppsrc/rpc/mempool.cppsrc/rpc/mining.cppsrc/rpc/rawtransaction.cppsrc/rpc/txoutproof.cppsrc/test/block_reward_reallocation_tests.cppsrc/test/blockfilter_index_tests.cppsrc/test/denialofservice_tests.cppsrc/test/dynamic_activation_thresholds_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/fuzz/mempool_utils.hsrc/test/fuzz/policy_estimator.cppsrc/test/fuzz/policy_estimator_io.cppsrc/test/fuzz/tx_pool.cppsrc/test/fuzz/validation_load_mempool.cppsrc/test/mempool_tests.cppsrc/test/miner_tests.cppsrc/test/net_peer_connection_tests.cppsrc/test/txpackage_tests.cppsrc/test/util/mining.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/util/txmempool.cppsrc/test/util/txmempool.hsrc/test/validation_block_tests.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/txmempool.cppsrc/txmempool.hsrc/util/system.cppsrc/util/system.hsrc/validation.cppsrc/validation.htest/functional/mempool_expiry.pytest/functional/mempool_persist.pytest/lint/lint-circular-dependencies.pytest/lint/spelling.ignore-words.txt
💤 Files with no reviewable changes (3)
- src/init/common.h
- src/init/common.cpp
- src/bls/bls.cpp
Comment on lines
+1800
to
+1804
| if test "$build_experimental_kernel_lib" = "no"; then | ||
| AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib]); | ||
| else | ||
| AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"]) | ||
| fi |
There was a problem hiding this comment.
Error fires unconditionally when kernel library is disabled.
Lines 1800-1802 error if build_experimental_kernel_lib = "no" regardless of whether build_bitcoin_chainstate is enabled. This breaks valid configurations where users disable the kernel library but also don't want chainstate (e.g., ./configure --without-experimental-kernel-lib alone).
The error should only fire when chainstate is explicitly enabled and the kernel library is disabled.
🐛 Proposed fix
-if test "$build_experimental_kernel_lib" = "no"; then
-AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib]);
-else
- AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"])
-fi
+if test "$build_bitcoin_chainstate" = "yes" && test "$build_experimental_kernel_lib" = "no"; then
+ AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib])
+fi
+AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` around lines 1800 - 1804, The AC_MSG_ERROR currently triggers
whenever build_experimental_kernel_lib = "no" even if chainstate isn't
requested; change the logic so the error is emitted only when both
build_bitcoin_chainstate = "yes" and build_experimental_kernel_lib = "no".
Concretely, replace the top-level if that checks build_experimental_kernel_lib
with a conditional that tests both variables (e.g., if test
"$build_bitcoin_chainstate" = "yes" && test "$build_experimental_kernel_lib" =
"no"; then AC_MSG_ERROR(...); else AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE],
[test $build_bitcoin_chainstate = "yes"]) fi), leaving the AM_CONDITIONAL for
BUILD_BITCOIN_CHAINSTATE unchanged.
Comment on lines
+14
to
+19
| enum class SanityCheckError { | ||
| ERROR_ECC, | ||
| ERROR_BLS, | ||
| ERROR_RANDOM, | ||
| ERROR_CHRONO, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for chrono-related sanity checks or time validation functions
rg -n "chrono.*sanity|time.*sanity|epoch.*check" --type=cpp -C3Repository: dashpay/dash
Length of output: 1461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ERROR_CHRONO usages =="
rg -n "ERROR_CHRONO" -S --type=cpp --type=h --type=hpp -g'*.cpp' -g'*.h' -g'*.hpp' -g'*.cc' .
echo
echo "== SanityChecks declaration/definition =="
rg -n "SanityChecks" src/kernel/checks.h src/kernel/checks.cpp src/kernel/checks.cc src/kernel/checks.h.*
# Show enum + function area in checks.h (small window)
echo
echo "== checks.h enum/function context =="
if [ -f src/kernel/checks.h ]; then
sed -n '1,120p' src/kernel/checks.h | nl -ba | sed -n '1,120p'
fi
# Show implementation in checks.cpp (small window around matches)
echo
echo "== checks.cpp SanityChecks implementation context =="
if [ -f src/kernel/checks.cpp ]; then
rg -n "SanityChecks" src/kernel/checks.cpp
# Print region around the first match
line=$(rg -n "SanityChecks" src/kernel/checks.cpp | head -n1 | cut -d: -f1)
start=$(( line>60 ? line-60 : 1 ))
end=$(( line+180 ))
sed -n "${start},${end}p" src/kernel/checks.cpp | nl -ba | sed -n '1,260p'
elif [ -f src/kernel/checks.cc ]; then
rg -n "SanityChecks" src/kernel/checks.cc
fi
# Show init.cpp switch area around referenced lines
echo
echo "== init.cpp around 1491-1493 =="
if [ -f src/init.cpp ]; then
sed -n '1460,1520p' src/init.cpp | nl -ba | sed -n '1,120p'
fiRepository: dashpay/dash
Length of output: 128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ERROR_CHRONO usages (cpp/h/cc only) =="
rg -n "ERROR_CHRONO" -S src -g'*.cpp' -g'*.h' -g'*.cc' || true
echo
echo "== SanityChecks declaration/definition =="
ls -1 src/kernel | rg -n "checks\.(h|cpp|cc)$" || true
rg -n "SanityChecks" -S src/kernel/checks.* src/kernel/*checks* || true
echo
echo "== checks.h enum context =="
if [ -f src/kernel/checks.h ]; then
nl -ba src/kernel/checks.h | sed -n '1,120p'
fi
echo
echo "== checks.cpp SanityChecks context =="
if [ -f src/kernel/checks.cpp ]; then
rg -n "SanityChecks" src/kernel/checks.cpp
line=$(rg -n "SanityChecks" src/kernel/checks.cpp | head -n1 | cut -d: -f1)
start=$(( line>80 ? line-80 : 1 ))
end=$(( line+220 ))
nl -ba src/kernel/checks.cpp | sed -n "${start},${end}p"
elif [ -f src/kernel/checks.cc ]; then
rg -n "SanityChecks" src/kernel/checks.cc
fi
echo
echo "== init.cpp around 1491-1493 =="
if [ -f src/init.cpp ]; then
nl -ba src/init.cpp | sed -n '1460,1525p'
fiRepository: dashpay/dash
Length of output: 710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== checks.h (first 80 lines) =="
if [ -f src/kernel/checks.h ]; then
cat -n src/kernel/checks.h | sed -n '1,80p'
fi
echo
echo "== checks.cpp (show SanityChecks implementation region) =="
if [ -f src/kernel/checks.cpp ]; then
rg -n "SanityChecks\\(const Context&\\)" src/kernel/checks.cpp || true
# print beginning plus function body area
cat -n src/kernel/checks.cpp | sed -n '1,220p'
fi
echo
echo "== init.cpp (around ERROR_CHRONO handling and surrounding switch) =="
if [ -f src/init.cpp ]; then
cat -n src/init.cpp | sed -n '1460,1525p'
fi
echo
echo "== Explicit searches inside checks.cpp for ERROR_CHRONO =="
if [ -f src/kernel/checks.cpp ]; then
rg -n "ERROR_CHRONO" src/kernel/checks.cpp || true
fiRepository: dashpay/dash
Length of output: 5133
Fix unreachable ERROR_CHRONO handling by implementing the missing chrono sanity check or removing the dead enum case.
kernel::SanityChecks() in src/kernel/checks.cpp only returns ERROR_ECC, ERROR_BLS, or ERROR_RANDOM (otherwise std::nullopt) and never returns ERROR_CHRONO, but AppInitSanityChecks in src/init.cpp still handles kernel::SanityCheckError::ERROR_CHRONO, making that switch branch unreachable/misleading. Consider either implementing the chrono check to return ERROR_CHRONO or dropping ERROR_CHRONO from SanityCheckError/the switch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/kernel/checks.h` around lines 14 - 19, The enum value ERROR_CHRONO is
unreachable because kernel::SanityChecks() never returns it while
AppInitSanityChecks still handles it; fix by either implementing the missing
chrono sanity check in kernel::SanityChecks() so it can return
SanityCheckError::ERROR_CHRONO (add a chrono validity test and return that enum
on failure), or remove ERROR_CHRONO from the SanityCheckError enum and delete
the corresponding case in AppInitSanityChecks (and update any switch/default
logic). Ensure the chosen change keeps enum and switch consistent and
compile-clean by updating any related tests/comments referencing ERROR_CHRONO.
| if (!is_coinsview_empty(chainstate)) { | ||
| const CBlockIndex* tip = chainstate->m_chain.Tip(); | ||
| if (tip && tip->nTime > get_unix_time_seconds() + MAX_FUTURE_BLOCK_TIME) { | ||
| if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) { |
There was a problem hiding this comment.
Honor the injected time callback here.
VerifyLoadedChainstate() still takes get_unix_time_seconds, but this comparison now bypasses it and hardcodes GetTime(). That makes custom/mock time sources ineffective and reintroduces the global timedata dependency this backport is supposed to remove.
Suggested fix
- if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
+ const int64_t now = get_unix_time_seconds();
+ if (tip && tip->nTime > now + MAX_FUTURE_BLOCK_TIME) {
return ChainstateLoadVerifyError::ERROR_BLOCK_FROM_FUTURE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) { | |
| const int64_t now = get_unix_time_seconds(); | |
| if (tip && tip->nTime > now + MAX_FUTURE_BLOCK_TIME) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/node/chainstate.cpp` at line 251, The comparison in
VerifyLoadedChainstate uses GetTime() directly, bypassing the injected time
callback; replace the hardcoded GetTime() call with the provided
get_unix_time_seconds() callback so the check if (tip && tip->nTime > ...)
becomes consistent with the injected time source (update the code in
VerifyLoadedChainstate where tip->nTime is compared to GetTime() +
MAX_FUTURE_BLOCK_TIME to use get_unix_time_seconds() instead).
Comment on lines
+120
to
+131
| CTxMemPool MakeMempool(const NodeContext& node) | ||
| { | ||
| // Take the default options for tests... | ||
| CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; | ||
|
|
||
| // ...override specific options for this specific fuzz suite | ||
| mempool_opts.estimator = nullptr; | ||
| mempool_opts.check_ratio = 1; | ||
|
|
||
| // ...and construct a CTxMemPool from it | ||
| return CTxMemPool{mempool_opts}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'struct MockedTxPool|CTxMemPool tx_pool_\{MakeMempool\(node\)\}|static_cast<MockedTxPool\*>\(&tx_pool_\)' src/test/fuzz/tx_pool.cppRepository: dashpay/dash
Length of output: 666
Fix UB from downcasting CTxMemPool to MockedTxPool in tx_pool fuzz test.
tx_pool_ is declared as CTxMemPool (CTxMemPool tx_pool_{MakeMempool(node)};), but then it’s cast to MockedTxPool& via *static_cast<MockedTxPool*>(&tx_pool_) (also repeated at the second block around lines ~330-332). This is UB unless the object was actually constructed as MockedTxPool, but MakeMempool() constructs/returns a CTxMemPool.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/fuzz/tx_pool.cpp` around lines 120 - 131, The code currently
constructs a CTxMemPool via MakeMempool but then UB-ly downcasts tx_pool_ to
MockedTxPool with *static_cast<MockedTxPool*>(&tx_pool_); fix this by actually
constructing a MockedTxPool instead of a CTxMemPool where the cast is used:
either change the declaration tx_pool_ to be MockedTxPool (and update
MakeMempool to return a MockedTxPool or add a MakeMockedMempool helper that
builds the same Options and constructs a MockedTxPool), and remove the
static_cast usages; do not leave a CTxMemPool and cast it to MockedTxPool.
Ensure references to CTxMemPool, MockedTxPool, tx_pool_, MakeMempool and the
static_cast sites are updated consistently.
Comment on lines
+20
to
+24
| class MemPoolTest final : public CTxMemPool | ||
| { | ||
| public: | ||
| using CTxMemPool::GetMinFee; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -p "setup_common.cpp" src/test src --exec rg -n -C3 'm_node\.mempool\s*=' {}
fd -p "setup_common.h" src/test src --exec rg -n -C3 'mempool' {}
rg -n -C2 'class MemPoolTest|static_cast<MemPoolTest&>\(\*Assert\(m_node\.mempool\)\)' src/test/mempool_tests.cppRepository: dashpay/dash
Length of output: 5973
Avoid UB: don’t cast m_node.mempool to MemPoolTest& when it’s a CTxMemPool.
At line 433, m_node.mempool is instantiated as std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node)), so static_cast<MemPoolTest&>(*Assert(m_node.mempool)) requires the dynamic type to be MemPoolTest; otherwise the behavior is undefined.
Fix by constructing a real MemPoolTest instance (using the same MemPoolOptionsForTest(m_node)) and using that object in the test, or by changing the fixture to store a MemPoolTest rather than CTxMemPool.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/mempool_tests.cpp` around lines 20 - 24, The test currently
static_casts m_node.mempool to MemPoolTest&, which is UB because m_node.mempool
is actually a std::unique_ptr<CTxMemPool>; fix by constructing a real
MemPoolTest object instead of a CTxMemPool: replace the
std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node)) allocation with
std::make_unique<MemPoolTest>(MemPoolOptionsForTest(m_node)) so the dynamic type
matches MemPoolTest, or alternatively change the fixture/member type that holds
m_node.mempool from std::unique_ptr<CTxMemPool> to std::unique_ptr<MemPoolTest>
and initialize it with MemPoolOptionsForTest(m_node); ensure all usages that
rely on CTxMemPool still compile against the MemPoolTest subclass.
thepastaclaw
suggested changes
Jun 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all six agent findings against the worktree at 9a2c79d. One blocking configure regression in the new --with-experimental-kernel-lib check: the guard fires whenever the kernel lib is disabled even though dash-chainstate is opt-in and defaults to no, so a plain --with-experimental-kernel-lib=no aborts configure. Three small backport-cleanup leftovers (stale Dump/LoadMempool declarations in validation.h, duplicate common/bloom.h line in Makefile.am, dead init::UnsetGlobals comment in bitcoin-chainstate.cpp) and two harmless adaptation residues (unused get_unix_time_seconds parameter, unreachable ERROR_CHRONO enum value) are kept as suggestions/nitpicks.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 4 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `configure.ac`:
- [BLOCKING] configure.ac:1799-1805: --with-experimental-kernel-lib=no aborts configure even when dash-chainstate is not requested
The new guard at configure.ac:1800 fires on `build_experimental_kernel_lib = no` unconditionally, but `build_bitcoin_chainstate` defaults to `no` (line 777) and is also forced to `no` at line 1460. The result: any invocation of `./configure --with-experimental-kernel-lib=no` (or any future path that disables the kernel library) bails out with an error about dash-chainstate, even though dash-chainstate was never requested. The check must be gated on `build_bitcoin_chainstate = yes` so it only triggers when the user actually asked for dash-chainstate while explicitly disabling the kernel library. As a bonus, the `AM_CONDITIONAL` must be emitted unconditionally so automake does not see a conditionally-defined conditional.
In `src/validation.h`:
- [SUGGESTION] src/validation.h:1110-1116: Stale global LoadMempool/DumpMempool declarations left behind after move to kernel::
bitcoin#25487 moves DumpMempool/LoadMempool into the kernel:: namespace with new signatures (taking dump_path / load_path) and the upstream hunk removes the old declarations from validation.h. This Dash backport correctly relocates the definitions to src/kernel/mempool_persist.cpp and updates callers via `using kernel::DumpMempool; using kernel::LoadMempool;`, but validation.h still advertises the old global-namespace declarations together with the `FopenFn` alias:
using FopenFn = std::function<FILE*(const fs::path&, const char*)>;
bool DumpMempool(const CTxMemPool& pool, FopenFn = fsbridge::fopen, bool skip_file_commit = false);
bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn = fsbridge::fopen);
No definitions match these signatures anywhere in the tree, so any future TU that includes validation.h and calls `LoadMempool(pool, chainstate)` will compile and then fail at link time with an undefined reference instead of the clear compile error it should get. Drop the dead declarations (and the now-unused `FopenFn` alias if nothing else uses it) to match the upstream cleanup.
Comment on lines
1799
to
1805
| AC_MSG_CHECKING([whether to build experimental dash-chainstate]) | ||
| AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"]) | ||
| if test "$build_experimental_kernel_lib" = "no"; then | ||
| AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib]); | ||
| else | ||
| AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"]) | ||
| fi | ||
| AC_MSG_RESULT($build_bitcoin_chainstate) |
There was a problem hiding this comment.
🔴 Blocking: --with-experimental-kernel-lib=no aborts configure even when dash-chainstate is not requested
The new guard at configure.ac:1800 fires on build_experimental_kernel_lib = no unconditionally, but build_bitcoin_chainstate defaults to no (line 777) and is also forced to no at line 1460. The result: any invocation of ./configure --with-experimental-kernel-lib=no (or any future path that disables the kernel library) bails out with an error about dash-chainstate, even though dash-chainstate was never requested. The check must be gated on build_bitcoin_chainstate = yes so it only triggers when the user actually asked for dash-chainstate while explicitly disabling the kernel library. As a bonus, the AM_CONDITIONAL must be emitted unconditionally so automake does not see a conditionally-defined conditional.
Suggested change
| AC_MSG_CHECKING([whether to build experimental dash-chainstate]) | |
| AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"]) | |
| if test "$build_experimental_kernel_lib" = "no"; then | |
| AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib]); | |
| else | |
| AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"]) | |
| fi | |
| AC_MSG_RESULT($build_bitcoin_chainstate) | |
| AC_MSG_CHECKING([whether to build experimental dash-chainstate]) | |
| if test "$build_bitcoin_chainstate" = "yes" && test "$build_experimental_kernel_lib" = "no"; then | |
| AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib]) | |
| fi | |
| AM_CONDITIONAL([BUILD_BITCOIN_CHAINSTATE], [test $build_bitcoin_chainstate = "yes"]) | |
| AC_MSG_RESULT($build_bitcoin_chainstate) |
source: ['codex']
Comment on lines
196
to
+197
| common/bloom.h \ | ||
| common/bloom.h \ |
There was a problem hiding this comment.
💬 Nitpick: Duplicate common/bloom.h entry in BITCOIN_CORE_H
The header-sorting commit in this PR produced a duplicate: common/bloom.h \ is listed on both line 196 and line 197 of BITCOIN_CORE_H. It does not break the build but the second copy is dead and should be removed.
Suggested change
| common/bloom.h \ | |
| common/bloom.h \ | |
| coins.h \ | |
| common/bloom.h \ | |
| common/run_command.h \ |
source: ['claude']
| @@ -317,6 +293,4 @@ int main(int argc, char* argv[]) | |||
| // Tear down Dash kernel objects before init::UnsetGlobals(). | |||
There was a problem hiding this comment.
💬 Nitpick: Stale comment references removed init::UnsetGlobals()
This PR removes init::SetGlobals/init::UnsetGlobals (their work moved into kernel::Context's ctor/dtor), but the teardown comment in bitcoin-chainstate.cpp still says // Tear down Dash kernel objects before init::UnsetGlobals().. The referenced function no longer exists; teardown is now driven by the kernel_context RAII destructor. Refresh (or drop) the comment so it no longer points at a removed symbol.
Suggested change
| // Tear down Dash kernel objects before init::UnsetGlobals(). | |
| // Tear down Dash kernel objects before kernel_context's destructor runs. |
source: ['claude']
Comment on lines
248
to
+251
| for (CChainState* chainstate : chainman.GetAll()) { | ||
| if (!is_coinsview_empty(chainstate)) { | ||
| const CBlockIndex* tip = chainstate->m_chain.Tip(); | ||
| if (tip && tip->nTime > get_unix_time_seconds() + MAX_FUTURE_BLOCK_TIME) { | ||
| if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) { |
There was a problem hiding this comment.
💬 Nitpick: Unused get_unix_time_seconds parameter retained from partial bitcoin#25168 adaptation
bitcoin#25168 (fa4ee53) removed the std::function<int64_t()> get_unix_time_seconds parameter from VerifyLoadedChainstate and replaced its single use with GetTime(). This backport applied the body change (chainstate.cpp:251 uses GetTime()) but kept the parameter in both the declaration (chainstate.h:134) and definition (chainstate.cpp:239). All callers (init.cpp, bitcoin-chainstate.cpp, setup_common.cpp) still pass a static_cast<int64_t(*)()>(GetTime) that is ignored. Non-blocking — no missing prerequisite — but the dead-parameter signature diverges from upstream for no Dash-specific reason and can be tightened in a follow-up.
source: ['claude']
Comment on lines
+14
to
+19
| enum class SanityCheckError { | ||
| ERROR_ECC, | ||
| ERROR_BLS, | ||
| ERROR_RANDOM, | ||
| ERROR_CHRONO, | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: SanityCheckError::ERROR_CHRONO unreachable after BLS-substitution adaptation of bitcoin#25065
The Dash adaptation of bitcoin#25065 correctly substitutes BLSInit() (returning ERROR_BLS) for the upstream chrono check, but the ERROR_CHRONO enum value is still defined here and the corresponding case is preserved in init.cpp's switch. ChronoSanityCheck was never backported and is not referenced anywhere in src/, so ERROR_CHRONO is unreachable. Harmless, just clutter — can be dropped for cleanliness.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Delta between previously reviewed SHA 9a2c79d and current head a1acc28 is empty — no new source changes. Both reviewer agents independently re-validated all 6 prior findings against the current head and found them STILL VALID; I verified each one in the worktree. One blocking issue (configure.ac --with-experimental-kernel-lib=no guard), one suggestion (stale validation.h declarations), and four nitpicks are carried forward.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.h`:
- [SUGGESTION] src/validation.h:1110-1116: Stale global LoadMempool/DumpMempool declarations left behind after move to kernel::
Verified at a1acc28a — validation.h:1110-1116 still declares `using FopenFn = std::function<FILE*(const fs::path&, const char*)>;` plus `DumpMempool(const CTxMemPool&, FopenFn, bool)` and `LoadMempool(CTxMemPool&, CChainState&, FopenFn)` in the global namespace. bitcoin#25487 moved these into `kernel::` with new path-taking signatures and the upstream hunk removed these declarations from validation.h. This backport correctly relocated the definitions to src/kernel/mempool_persist.cpp and updates callers via `using kernel::DumpMempool; using kernel::LoadMempool;`, but the old global declarations remain advertised with no matching definitions. A future TU that includes validation.h and calls `LoadMempool(pool, chainstate)` will compile and then fail at link time with an undefined reference instead of producing a clear compile error. Drop the dead declarations and the now-unused `FopenFn` alias to match the upstream cleanup.
thepastaclaw
suggested changes
Jun 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review — correction / carry-forward
Correction: the review system briefly posted auto-resolve replies on several prior threads even though this recheck verified those findings are still valid. I removed those incorrect auto-resolve replies.
I re-reviewed current head a1acc28a4ba1737d8bba27498b3a9ea1dd288fe0 cumulatively and checked the requested delta git diff 9a2c79db..a1acc28a4ba1737d8bba27498b3a9ea1dd288fe0. The delta is empty, and all six prior findings remain valid at the current head:
- STILL VALID / blocking:
configure.ac—--with-experimental-kernel-lib=nostill aborts configure even whendash-chainstateis not requested. - STILL VALID / suggestion:
src/validation.h— stale globalLoadMempool/DumpMempooldeclarations remain after the move tokernel::. - STILL VALID / nitpick:
src/Makefile.am— duplicatecommon/bloom.hremains inBITCOIN_CORE_H. - STILL VALID / nitpick:
src/bitcoin-chainstate.cpp— stale comment still references removedinit::UnsetGlobals(). - STILL VALID / nitpick:
src/node/chainstate.cpp— unusedget_unix_time_secondsparameter remains from the partial bitcoin#25168 adaptation. - STILL VALID / nitpick:
src/kernel/checks.h— unreachableSanityCheckError::ERROR_CHRONOremains after the BLS-substitution adaptation of bitcoin#25065.
No new latest-delta findings were found. Requesting changes is still warranted because the carried-forward configure issue is blocking.
BACKPORT NOTE: build: Don't add unrelated libs to LIBTEST_* --- done via a3c11b6 035fa1f build: Remove LIBTOOL_APP_LDFLAGS for bitcoin-chainstate (Cory Fields) 3f05950 docs: Add libbitcoinkernel_la_SOURCES explanation (Carl Dong) 94ad45d ci: Build libbitcoinkernel (Carl Dong) 26b2e7f build: Extract the libbitcoinkernel library (Carl Dong) 1df44dd b-cs: Define G_TRANSLATION_FUN in bitcoinkernel.cpp (Carl Dong) 83a0bb7 build: Separate lib_LTLIBRARIES initialization (Carl Dong) c1e16cb build: Create .la library for bitcoincrypto (Carl Dong) 8bdfe05 build: Create .la library for leveldb (Carl Dong) 05d1525 build: Create .la library for crc32c (Carl Dong) 64caf94 build: Remove vestigial LIBLEVELDB_SSE42 (Carl Dong) 1392e8e build: Don't add unrelated libs to LIBTEST_* (Carl Dong) Pull request description: Part of: bitcoin#24303 This PR introduces a `libbitcoinkernel` static library linking in the minimal list of files necessary to use our consensus engine as-is. `bitcoin-chainstate` introduced in bitcoin#24304 now will link against `libbitcoinkernel`. Most of the changes are related to the build system. Please read the commit messages for more details. ACKs for top commit: theuni: This may be my favorite PR ever. It's a privilege to ACK 035fa1f. Tree-SHA512: b755edc3471c7c1098847e9b16ab182a6abb7582563d9da516de376a770ac7543c6fdb24238ddd4d3d2d458f905a0c0614b8667aab182aa7e6b80c1cca7090bc Co-authored-by: fanquake <fanquake@gmail.com>
…ter for burried deployments
fa1b76a Do not call global Params() when chainman is in scope (MacroFake) fa30234 Do not pass CChainParams& to PeerManager::make (MacroFake) fafe5c0 Do not pass CChainParams& to BlockAssembler constructor (MacroFake) faf012b Do not pass Consensus::Params& to Chainstate helpers (MacroFake) fa4ee53 Do not pass time getter to Chainstate helpers (MacroFake) Pull request description: It seems confusing to pass chain params, consensus params, or a time function around when it is not needed. Fix this by: * Inlining the passed time getter function. I don't see a use case why this should be mockable. * Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible. ACKs for top commit: promag: Code review ACK fa1b76a. vincenzopalazzo: ACK bitcoin@fa1b76a Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd Co-authored-by: MacroFake <falke.marco@gmail.com>
…adjusted_time_callback`
BACKPORT NOTE:
next dependencies are still there:
- netaddress.cpp
- netbase.cpp
- timedata.cpp
There's tiny script to find-out who is exactly blocker:
find . -name '*kernel*.o' -exec bash -c "nm -C -u '{}' 2>/dev/null | grep -qF 'GetSporkValue(' && echo '{}'" \;
GetAdjustedTime is used by governance/vote, governance/object, spork_manager
CNetAddress, CService are used by evo/netinfo
----
53494bc validation: Have ChainstateManager own m_chainparams (Carl Dong)
04c31c1 Add ChainstateManager::m_adjusted_time_callback (Carl Dong)
dbe45c3 Add ChainstateManagerOpts, using as ::Options (Carl Dong)
Pull request description:
```
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
```
Top commit has no ACKs.
Tree-SHA512: a093ec6ecacdc659d656574f05bd31ade6a6cdb64d5a97684f94ae7e55c0e360b78177553d4d1ef40280192674464d029a0d68e96caf8711d9274011172f1330
Co-authored-by: MacroFake <falke.marco@gmail.com>
…sulate global init/teardown d87784a kernel: SanityChecks: Return an error struct (Carl Dong) 265d639 Move init::SanityCheck to kernel::SanityCheck (Carl Dong) fed085a init: Initialize globals with kernel::Context's life (Carl Dong) 7d03fee kernel: Introduce empty and unused kernel::Context (Carl Dong) eeb4fc2 test: Use Set/UnsetGlobals in BasicTestingSetup (Carl Dong) Pull request description: The full `init/common.cpp` is dependent on things like ArgsManager (which we wish to remove from libbitcoinkernel in the future) and sanity checks. These aren't necessary for libbitcoinkernel so we only extract the portion that is necessary (namely `init::{Set,Unset}Globals()`. ACKs for top commit: theuni: ACK d87784a vasild: ACK d87784a Tree-SHA512: cd6b4923ea1865001b5f0caed9a4ff99c198d22bf74154d935dc09a47fda22ebe585ec912398cea69f722454ed1dbb4898faab5a2d02fb4c5e719c5c8d71a3f9 Co-authored-by: fanquake <fanquake@gmail.com>
…` to ignore-words d575413 doc: add `desig` to ignore-words (brunoerg) c06cc41 doc: fix typo in kernel/context.h (brunoerg) Pull request description: This PR fixes a typo in `kernel/context.h` (libary => library) and add `desig` to ignore-words since it's a valid word, see: https://github.com/bitcoin/bitcoin/blob/b9416c3847cd347238a9d75d949327f69e187d79/src/net.cpp#L1105-L1117 ACKs for top commit: fanquake: ACK d575413 Tree-SHA512: 2d548c737b8184d0243445c7503f3f68256ecb0970bd834d52de099de3cd8c8b9c140e2b77d55e2542fbd45b1d21cbdee639f5b2ef8138c37b8b72e5211029c3 Co-authored-by: fanquake <fanquake@gmail.com>
ce1c810 build: Remove unused `LIBBITCOIN_KERNEL` variable (Hennadii Stepanov) Pull request description: Noticed that while working on moving the build system to CMake. But I [am not the first](https://github.com/bitcoin/bitcoin/pull/24322/files#r860472867) one :) ACKs for top commit: laanwj: ACK ce1c810 Tree-SHA512: 877b9f0d64c4c72f403335d7a8462e551f6f8cd5648a211f980d6da5ed7683521d6549f6acf15ac8e55f67915c556201a1980228c975a22135507746e2f392ce Co-authored-by: laanwj <126646+laanwj@users.noreply.github.com>
…nager` BACKPORT NOTE: includes changes from Merge bitcoin#25223: [kernel 2e/n] miner: Make `mempool` optional, stop constructing temporary empty mempools includes changes from Merge bitcoin#26286: test: Remove unused txmempool include from tests d1684be fees: Pass in a filepath instead of referencing gArgs (Carl Dong) 9a3d825 init: Remove redundant -*mempool*, -limit* queries (Carl Dong) 6c5c60c mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong) 9e93b10 node/ifaces: Use existing MemPoolLimits (Carl Dong) 38af2bc mempoolaccept: Use limits from mempool in constructor (Carl Dong) 9333427 mempool: Introduce (still-unused) MemPoolLimits (Carl Dong) 716bb5f scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong) 1ecc773 scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong) aa9141c mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong) 51c7a41 init: Only determine maxmempool once (Carl Dong) 386c947 mempool: Make GetMinFee() with custom size protected (Carl Dong) 82f00de mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong) f1941e8 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong) 0199bd3 fuzz/rbf: Add missing TestingSetup (Carl Dong) ccbaf54 scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong) fc02f77 ArgsMan: Add Get*Arg functions returning optional (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: bitcoin#24303, https://github.com/bitcoin/bitcoin/projects/18 ----- As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](bitcoin#24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this. This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time. These options are: - `-maxmempool` -> `CTxMemPool::Options::max_size` - `-mempoolexpiry` -> `CTxMemPool::Options::expiry` - `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count` - `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size` - `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count` - `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size` More context can be gleaned from the commit messages. The important commits are: - 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions" - a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs" - 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs" - 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits" Reviewers: Help needed in the following commits (see commit messages): - a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs" - 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits" Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch. ----- TODO: - [x] Use the more ergonomic `CTxMemPool::Options` where appropriate - [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions` ----- Questions for Reviewers: 1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?) 2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`? ACKs for top commit: MarcoFalke: ACK d1684be 🍜 ryanofsky: Code review ACK d1684be. Just minor cleanups since last review, mostly switching to brace initialization Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b Co-authored-by: MacroFake <falke.marco@gmail.com>
…fixups BACKPORT NOTE: changes in src/test/validation_chainstate_tests.cpp is skipped as relevant code is removed by bitcoin#25815 ce8b0f9 Use designated initializers for ChainstateManager::Options (Carl Dong) 3837700 Move ChainstateManagerOpts into kernel:: namespace (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: bitcoin#24303, https://github.com/bitcoin/bitcoin/projects/18 This PR is **_NOT_** dependent on any other PRs. ----- Places `ChainstateManager::Options` into the `kernel::` namespace and use designated initializers for construction. ACKs for top commit: ryanofsky: Code review ACK ce8b0f9 Tree-SHA512: 16a11b5051a2432ca4b6fa7b253376606fef619ace499dfe64d033c8fbe3e1a1875a7c946d7cd54bd908363886244ddf3a192e2f0c801ffbed40d60aad65e442 Co-authored-by: MacroFake <falke.marco@gmail.com>
… `ArgsManager` cb3e9a1 Move {Load,Dump}Mempool to kernel namespace (Carl Dong) aa30676 Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel (Carl Dong) 06b88ff LoadMempool: Pass in load_path, stop using gArgs (Carl Dong) b857ac6 test/fuzz: Invoke LoadMempool via CChainState (Carl Dong) b326725 Move FopenFn to fsbridge namespace (Carl Dong) ae1e8e3 mempool: Use NodeClock+friends for LoadMempool (Carl Dong) f9e8e57 mempool: Improve comments for [GS]etLoadTried (Carl Dong) 813962d scripted-diff: Rename m_is_loaded -> m_load_tried (Carl Dong) 413f4bb DumpMempool: Pass in dump_path, stop using gArgs (Carl Dong) bd44078 DumpMempool: Use std::chrono instead of weird int64_t arthmetics (Carl Dong) c84390b test/mempool_persist: Test manual savemempool when -persistmempool=0 (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: bitcoin#24303, https://github.com/bitcoin/bitcoin/projects/18 ----- This PR moves `{Dump,Load}Mempool` into its own `kernel/mempool_persist` module and introduces `ArgsManager` `node::` helpers in `node/mempool_persist_args`to remove the scattered calls to `GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)`. More context can be gleaned from the commit messages. ----- One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since `libbitcoinkernel` will include both validation and mempool, but perhaps something for the future. ACKs for top commit: glozow: re ACK cb3e9a1 via `git range-diff 7ae032e...cb3e9a1` MarcoFalke: ACK cb3e9a1 🔒 ryanofsky: Code review ACK cb3e9a1 Tree-SHA512: 979d7237c3abb5a1dd9b5ad3dbf3b954f906a6d8320ed7b923557f41a4472deccae3e8a6bca0018c8e7a3c4a93afecc502acd1e26756f2054f157f1c0edd939d Co-authored-by: glozow <gloriajzhao@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 461435620a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+1800
to
+1801
| if test "$build_experimental_kernel_lib" = "no"; then | ||
| AC_MSG_ERROR([experimental dash-chainstate cannot be built without the experimental dashkernel library. Use --with-experimental-kernel-lib]); |
There was a problem hiding this comment.
When a user explicitly disables the experimental kernel library with --without-experimental-kernel-lib while leaving --enable-experimental-util-chainstate at its default no, configure still enters this branch and aborts. The dependency error should only be raised when chainstate was requested; otherwise a normal opt-out of the experimental library makes non-chainstate builds fail unnecessarily.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/validation.cpp (1)
4342-4379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
bls_legacy_schemeon every exit path.
TestBlockValidity()snapshots the global flag, but every failure return before the tail restore skips putting it back. If any of the validation calls in this function flip the scheme and then fail, later validations inherit the wrong global state. Please move the restore into a scope-exit/RAII guard so it runs on both success and error paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validation.cpp` around lines 4342 - 4379, TestBlockValidity currently snapshots bls::bls_legacy_scheme into a local bls_legacy_scheme but only restores it at the end, skipping all early returns; wrap that snapshot in an RAII/scope-exit guard so the original value is restored on every exit (success or any early return) — e.g. after auto bls_legacy_scheme = bls::bls_legacy_scheme.load(); create a small scope_guard that calls bls::bls_legacy_scheme.store(bls_legacy_scheme) in its destructor, remove the manual tail restore block that checks and stores the value, and leave all existing checks (ContextualCheckBlockHeader, CheckBlock, ContextualCheckBlock, chainstate.ConnectBlock, etc.) unchanged so the guard handles restoration even when those calls return early.
♻️ Duplicate comments (1)
src/test/fuzz/tx_pool.cpp (1)
120-131:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConstructing
CTxMemPoolthen downcasting toMockedTxPoolis UB.Line 120 returns a base
CTxMemPool, but Line 155 and Line 331 cast those objects toMockedTxPool&. That downcast is invalid unless the object was originally constructed asMockedTxPool.Suggested fix
struct MockedTxPool : public CTxMemPool { + using CTxMemPool::CTxMemPool; + void RollingFeeUpdate() EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } }; @@ -CTxMemPool MakeMempool(const NodeContext& node) +MockedTxPool MakeMempool(const NodeContext& node) { @@ - return CTxMemPool{mempool_opts}; + return MockedTxPool{mempool_opts}; } @@ - CTxMemPool tx_pool_{MakeMempool(node)}; - MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_); + MockedTxPool tx_pool{MakeMempool(node)}; @@ - CTxMemPool tx_pool_{MakeMempool(node)}; - MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_); + MockedTxPool tx_pool{MakeMempool(node)};#!/bin/bash set -euo pipefail # Verify the base construction + derived downcast pattern is present. rg -n -C2 'MockedTxPool|MakeMempool\(|static_cast<MockedTxPool\*>\(&tx_pool_\)' src/test/fuzz/tx_pool.cppAlso applies to: 154-156, 330-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/fuzz/tx_pool.cpp` around lines 120 - 131, Make MakeMempool construct and return a polymorphic heap object (avoid slicing) instead of returning CTxMemPool by value: change the signature of MakeMempool to return std::unique_ptr<CTxMemPool> and return std::make_unique<MockedTxPool>(mempool_opts) (use MockedTxPool's constructor that accepts mempool_opts); then update call sites that currently take the returned value (and that do static_cast<MockedTxPool*>(&tx_pool_) or static_cast<MockedTxPool&>(...)) to hold the std::unique_ptr and either dynamic_cast the pointer to MockedTxPool* (and check for null) or otherwise use the CTxMemPool virtual interface so no downcast is needed—this ensures MakeMempool, CTxMemPool, MockedTxPool, and places referencing tx_pool_ are consistent and avoid undefined behavior.
🧹 Nitpick comments (1)
src/kernel/context.cpp (1)
25-27: BLSInit() is safe to call twice (allocator callback assignment), but the sanity-check error path is redundant
kernel::Context::Context()callsBLSInit()(src/kernel/context.cpp:25-27), andkernel::SanityChecks()calls it again (src/kernel/checks.cpp:19-20).BLSInit()(src/bls/bls.cpp:431-437) only invokesbls::BLS::SetSecureAllocator(secure_allocate, secure_free)and unconditionally returnstrue; it does not perform the one-time relic/BLS library initialization.bls::BLS::SetSecureAllocator(src/dashbls/src/bls.cpp:72-78) only assigns global callback function pointers, so repeated calls just overwrite the same callbacks.- One-time BLS library init happens separately via
bool BLSInitResult = BLS::Init();(src/dashbls/src/bls.cpp:29), so the kernel startup path isn’t duplicating that init.Optional cleanup: since
BLSInit()can’t fail, theif (!BLSInit()) { ... ERROR_BLS }branch insrc/kernel/checks.cppis unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kernel/context.cpp` around lines 25 - 27, The constructor call to BLSInit() in src/kernel/context.cpp (lines 25-27) is fine and requires no change; remove the redundant BLSInit() invocation and its failing-branch in src/kernel/checks.cpp (lines 19-20) — delete the if (!BLSInit()) { ... ERROR_BLS } block (or at minimum the BLSInit() call and related error handling) since BLSInit() cannot fail and the one-time library init occurs elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/kernel/checks.cpp`:
- Around line 13-27: SanityChecks currently never returns
SanityCheckError::ERROR_CHRONO; update the SanityChecks(const Context&)
implementation to invoke the chrono sanity check and return ERROR_CHRONO on
failure (e.g., call the existing chrono-check function — name it
ChronoSanityCheck() or the actual project function that validates
monotonic/chrono behavior — and if it returns false, return
SanityCheckError::ERROR_CHRONO); alternatively, if no chrono check exists,
remove the ERROR_CHRONO enum and any caller handling (e.g., in src/init.cpp) so
the public contract and callers stay consistent.
---
Outside diff comments:
In `@src/validation.cpp`:
- Around line 4342-4379: TestBlockValidity currently snapshots
bls::bls_legacy_scheme into a local bls_legacy_scheme but only restores it at
the end, skipping all early returns; wrap that snapshot in an RAII/scope-exit
guard so the original value is restored on every exit (success or any early
return) — e.g. after auto bls_legacy_scheme = bls::bls_legacy_scheme.load();
create a small scope_guard that calls
bls::bls_legacy_scheme.store(bls_legacy_scheme) in its destructor, remove the
manual tail restore block that checks and stores the value, and leave all
existing checks (ContextualCheckBlockHeader, CheckBlock, ContextualCheckBlock,
chainstate.ConnectBlock, etc.) unchanged so the guard handles restoration even
when those calls return early.
---
Duplicate comments:
In `@src/test/fuzz/tx_pool.cpp`:
- Around line 120-131: Make MakeMempool construct and return a polymorphic heap
object (avoid slicing) instead of returning CTxMemPool by value: change the
signature of MakeMempool to return std::unique_ptr<CTxMemPool> and return
std::make_unique<MockedTxPool>(mempool_opts) (use MockedTxPool's constructor
that accepts mempool_opts); then update call sites that currently take the
returned value (and that do static_cast<MockedTxPool*>(&tx_pool_) or
static_cast<MockedTxPool&>(...)) to hold the std::unique_ptr and either
dynamic_cast the pointer to MockedTxPool* (and check for null) or otherwise use
the CTxMemPool virtual interface so no downcast is needed—this ensures
MakeMempool, CTxMemPool, MockedTxPool, and places referencing tx_pool_ are
consistent and avoid undefined behavior.
---
Nitpick comments:
In `@src/kernel/context.cpp`:
- Around line 25-27: The constructor call to BLSInit() in src/kernel/context.cpp
(lines 25-27) is fine and requires no change; remove the redundant BLSInit()
invocation and its failing-branch in src/kernel/checks.cpp (lines 19-20) —
delete the if (!BLSInit()) { ... ERROR_BLS } block (or at minimum the BLSInit()
call and related error handling) since BLSInit() cannot fail and the one-time
library init occurs elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a24de521-92cd-4f11-ac40-1d970d8854a3
📒 Files selected for processing (88)
ci/dash/lint-tidy.shci/test/00_setup_env_native_nowallet_libbitcoinkernel.shconfigure.accontrib/devtools/iwyu/bitcoin.core.impsrc/Makefile.amsrc/Makefile.test_fuzz.includesrc/bitcoin-chainstate.cppsrc/bitcoind.cppsrc/bls/bls.hsrc/bls/bls_worker.cppsrc/bls/bls_worker.hsrc/deploymentstatus.hsrc/evo/mnhftx.cppsrc/fs.hsrc/init.cppsrc/init.hsrc/init/common.cppsrc/init/common.hsrc/kernel/bitcoinkernel.cppsrc/kernel/chainstatemanager_opts.hsrc/kernel/checks.cppsrc/kernel/checks.hsrc/kernel/context.cppsrc/kernel/context.hsrc/kernel/mempool_limits.hsrc/kernel/mempool_options.hsrc/kernel/mempool_persist.cppsrc/kernel/mempool_persist.hsrc/mempool_args.cppsrc/mempool_args.hsrc/net_processing.cppsrc/net_processing.hsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/context.cppsrc/node/context.hsrc/node/interfaces.cppsrc/node/mempool_persist_args.cppsrc/node/mempool_persist_args.hsrc/node/miner.cppsrc/node/miner.hsrc/policy/fees.cppsrc/policy/fees.hsrc/policy/fees_args.cppsrc/policy/fees_args.hsrc/policy/packages.hsrc/policy/policy.hsrc/rest.cppsrc/rpc/blockchain.cppsrc/rpc/fees.cppsrc/rpc/mempool.cppsrc/rpc/mining.cppsrc/rpc/rawtransaction.cppsrc/rpc/txoutproof.cppsrc/test/block_reward_reallocation_tests.cppsrc/test/blockfilter_index_tests.cppsrc/test/denialofservice_tests.cppsrc/test/dynamic_activation_thresholds_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/fuzz/mempool_utils.hsrc/test/fuzz/policy_estimator.cppsrc/test/fuzz/policy_estimator_io.cppsrc/test/fuzz/tx_pool.cppsrc/test/fuzz/validation_load_mempool.cppsrc/test/mempool_tests.cppsrc/test/miner_tests.cppsrc/test/net_peer_connection_tests.cppsrc/test/txpackage_tests.cppsrc/test/util/mining.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/util/txmempool.cppsrc/test/util/txmempool.hsrc/test/validation_block_tests.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/txmempool.cppsrc/txmempool.hsrc/util/system.cppsrc/util/system.hsrc/validation.cppsrc/validation.htest/functional/mempool_expiry.pytest/functional/mempool_persist.pytest/lint/lint-circular-dependencies.pytest/lint/spelling.ignore-words.txt
💤 Files with no reviewable changes (3)
- src/init/common.h
- src/init/common.cpp
- src/bls/bls.h
✅ Files skipped from review due to trivial changes (10)
- test/lint/spelling.ignore-words.txt
- src/node/context.cpp
- ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh
- ci/dash/lint-tidy.sh
- contrib/devtools/iwyu/bitcoin.core.imp
- src/evo/mnhftx.cpp
- src/policy/packages.h
- src/test/validation_chainstate_tests.cpp
- src/policy/fees_args.cpp
- test/functional/mempool_expiry.py
🚧 Files skipped from review as they are similar to previous changes (50)
- src/test/fuzz/mempool_utils.h
- src/kernel/checks.h
- src/test/mempool_tests.cpp
- src/test/util/mining.cpp
- src/test/miner_tests.cpp
- src/kernel/mempool_limits.h
- src/Makefile.test_fuzz.include
- src/rpc/txoutproof.cpp
- src/rest.cpp
- src/mempool_args.h
- src/node/blockstorage.cpp
- src/node/blockstorage.h
- src/kernel/context.h
- src/test/txpackage_tests.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/bitcoind.cpp
- src/test/validation_block_tests.cpp
- test/lint/lint-circular-dependencies.py
- src/init.h
- src/deploymentstatus.h
- src/test/util/txmempool.h
- src/net_processing.h
- src/policy/fees.h
- src/node/miner.h
- src/test/denialofservice_tests.cpp
- src/fs.h
- src/util/system.h
- src/kernel/mempool_persist.h
- src/kernel/chainstatemanager_opts.h
- src/node/miner.cpp
- src/policy/fees_args.h
- src/rpc/blockchain.cpp
- src/kernel/mempool_options.h
- src/node/context.h
- src/test/evo_deterministicmns_tests.cpp
- src/util/system.cpp
- src/policy/policy.h
- src/node/chainstate.h
- configure.ac
- src/net_processing.cpp
- src/policy/fees.cpp
- src/txmempool.h
- src/txmempool.cpp
- src/node/interfaces.cpp
- src/node/chainstate.cpp
- src/validation.h
- src/rpc/fees.cpp
- src/rpc/mining.cpp
- src/init.cpp
- src/Makefile.am
Comment on lines
+13
to
+27
| std::optional<SanityCheckError> SanityChecks(const Context&) | ||
| { | ||
| if (!ECC_InitSanityCheck()) { | ||
| return SanityCheckError::ERROR_ECC; | ||
| } | ||
|
|
||
| if (!BLSInit()) { | ||
| return SanityCheckError::ERROR_BLS; | ||
| } | ||
|
|
||
| if (!Random_SanityCheck()) { | ||
| return SanityCheckError::ERROR_RANDOM; | ||
| } | ||
|
|
||
| return std::nullopt; |
There was a problem hiding this comment.
ERROR_CHRONO is never produced.
kernel/checks.h publishes SanityCheckError::ERROR_CHRONO, and src/init.cpp:1473-1501 already treats it as a real startup failure, but this implementation can only return ECC/BLS/RANDOM. That silently drops one branch of the new kernel sanity-check contract instead of surfacing it through the advertised API. Add the missing chrono sanity check here, or remove the enum value and caller handling so the contract stays honest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/kernel/checks.cpp` around lines 13 - 27, SanityChecks currently never
returns SanityCheckError::ERROR_CHRONO; update the SanityChecks(const Context&)
implementation to invoke the chrono sanity check and return ERROR_CHRONO on
failure (e.g., call the existing chrono-check function — name it
ChronoSanityCheck() or the actual project function that validates
monotonic/chrono behavior — and if it returns false, return
SanityCheckError::ERROR_CHRONO); alternatively, if no chrono check exists,
remove the ERROR_CHRONO enum and any caller handling (e.g., in src/init.cpp) so
the public contract and callers stay consistent.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #7361 at 4614356. The latest delta (a1acc28..4614356) is a small housekeeping pass — wrapping Dash BLS APIs in #ifndef BUILD_BITCOIN_INTERNAL for libdashkernel exclusion and removing one stale using node::ShouldPersistMempool in rpc/mempool.cpp. No new defects in the delta. All six prior findings from a1acc28 remain unaddressed and are carried forward verbatim. The blocking issue remains configure.ac aborting when --with-experimental-kernel-lib=no is passed even without dash-chainstate being requested.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.h`:
- [SUGGESTION] src/validation.h:1110-1116: Stale global LoadMempool/DumpMempool declarations left behind after move to kernel::
Carried forward — STILL VALID at 461435620a. Verified: validation.h:1110-1116 still declares `using FopenFn = std::function<FILE*(const fs::path&, const char*)>;` plus `DumpMempool(const CTxMemPool&, FopenFn, bool)` and `LoadMempool(CTxMemPool&, CChainState&, FopenFn)` in the global namespace. bitcoin#25487 moved these into kernel:: with new path-taking signatures (definitions now live at src/kernel/mempool_persist.cpp); the upstream hunk removed these prototypes from validation.h. Callers in-tree correctly use `using kernel::DumpMempool; using kernel::LoadMempool;`, but the old globals are still advertised with no matching definitions. A future translation unit including validation.h that calls `LoadMempool(pool, chainstate)` will compile successfully and then fail at link time with an undefined reference instead of producing a clear compile error. Drop the dead declarations and the now-unused FopenFn alias, and have callers include kernel/mempool_persist.h directly.
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
This PR is a batch of backports of the libdashkernel project, see for details: bitcoin#24303
What was done?
This PR introduces a
libdashkernelstatic library linking in the minimal list of files necessary to use our consensus engine as-is.dash-chainstateintroduced in #24304 now will link againstlibdashkernel.Beside that other backports are done:
kernel::Context, encapsulate global init/teardown bitcoin/bitcoin#25065)CTxMemPoolfromArgsManagerbitcoin/bitcoin#25290)ChainstateManager::Optionsfixups bitcoin/bitcoin#25607){Dump,Load}MempoolfromArgsManagerbitcoin/bitcoin#25487)How Has This Been Tested?
src/dash-chainstate -datadir=/.dashcorestill succeed.Unit & functional tests succeed.
Breaking Changes
N/A
Checklist: