Repository navigation
Conversation
Default stays 190000 so existing clients keep the Core 0.19 response shape. --rpc-client-version N (conf rpc_client_version, range 1..=1000000) reports a higher integer without adding methods. Bark captaind requires 310000 because it reads getmempoolentry chunk fields this node already returns.
An output of a mempool transaction came back from gettxout even when a second mempool transaction already spent it. Core's mempool coins view drops it, the same as the confirmed branch here already does. A wallet that picks coins with gettxout(include_mempool=true) double-spent its own unconfirmed change and got "insufficient fee" from the RBF check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A block off the active chain (confirmations -1) only carried size, strippedsize and weight at verbosity 2. Core sends them from verbosity 1 for any block. BDK's bitcoind emitter reads verbose getblock while it walks a reorg and fails on the missing field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Core's getmempoolentry and verbose getrawmempool carry bip125-replaceable: true when the tx or an unconfirmed ancestor has an input sequence at or below 0xfffffffd. rust-bitcoin's GetMempoolEntryResult requires the field, so clients built on bitcoincore-rpc fail to parse an rbitcoin entry without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
generate, generatetoaddress, generatetodescriptor and generateblock selected pool transactions and then connected their block with no lock across the two. Two calls at once could both select a transaction; the second block then carried one the first had just confirmed and was rejected as bad-txns-BIP30. Core assembles and connects under cs_main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
coreyphillips
left a comment
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
needs changing before merge
- CHANGELOG.md edited on a feature branch (CHANGELOG.md:16-18)
worth doing, does not block
- Mining lock comment misstates Core, and the lock duplicates the hub's (crates/rbitcoin-rpc/src/methods/mine.rs:37-47)
- docs/rpc.md still describes the old gettxout and getmempoolentry output (docs/rpc.md:147). The
gettxoutrow at docs/rpc.md:167 still says only a confirmed output spent by a mempool tx is null, and that "A mempool tx's outputs are all listed, as in Core". After this PR, a mempool output that another mempool tx spends is also null. Thegetmempoolentry/getrawmempoolrow at :147 lists the entry fields but notbip125-replaceable. That row also saysfullrbfistrue(Libre always replaces), so it should say the new field reports BIP 125 signaling (on the tx or an unconfirmed ancestor), not whether this node would accept a replacement. rpc.md owns these facts per docs/README.md.
nits
- Test doc comment now attaches to the assert_bip125 helper (crates/rbitcoin-rpc/tests/methods/mod.rs:1236-1243)
- signals_bip125 clones every ancestor body (crates/rbitcoin-rpc/src/methods/mempool.rs:144-162)
| - **`getnetworkinfo.version` stays 190000 by default.** `--rpc-client-version N` | ||
| (conf `rpc_client_version=`, range 1–1000000) raises only that integer. | ||
| Response shapes do not change. Bark captaind needs 310000. |
There was a problem hiding this comment.
needs changing before merge: CHANGELOG.md edited on a feature branch
Commit 62a331a adds the --rpc-client-version bullet straight into ## [Unreleased] in CHANGELOG.md. The other four commits correctly add changelog.d/ fragments. AGENTS.md lists "Do not edit CHANGELOG.md on a feature branch" as a hard stop. docs/releases.md:48 says feature pulls do not edit it and that unreleased notes are one changelog.d/<topic>.md per pull, so that open branches do not collide on the same line. I confirmed it with git diff origin/master...HEAD -- CHANGELOG.md. The fix is to revert CHANGELOG.md and add something like changelog.d/rpc-client-version.md with category Added, since this is a new flag.
| /// One regtest mining call at a time, from pool selection through the block | ||
| /// connect. Core assembles and connects under cs_main; without this, a second | ||
| /// call can select a transaction the first just confirmed and mine it again | ||
| /// (`bad-txns-BIP30`). | ||
| static REGTEST_MINING: std::sync::Mutex<()> = std::sync::Mutex::new(()); | ||
|
|
||
| fn regtest_mining_lock() -> std::sync::MutexGuard<'static, ()> { | ||
| REGTEST_MINING | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner) | ||
| } |
There was a problem hiding this comment.
worth doing, does not block: Mining lock comment misstates Core, and the lock duplicates the hub's
The comment at mine.rs:37-40 and the changelog fragment say Core assembles and connects under cs_main. It does not. In Core, generateBlocks calls createNewBlock, which takes cs_main and mempool.cs together, and only afterwards calls ProcessNewBlock. generateblock releases cs_main before GenerateBlock. Taking both locks together keeps each template's tip and transaction set consistent, so a losing concurrent call in Core produces a stale sibling block, not a bad-txns-BIP30 failure. The actual rbitcoin bug is that select_generate_txs runs outside the hub's existing generate_lock (chain.rs:2401), which already serializes tip read, mining and accept. This PR adds a static REGTEST_MINING mutex around that one. Because it is process-global rather than per hub, an in-process multi-hub harness now serializes mining across unrelated nodes. It also does not cover a peer block or submitblock that confirms the selected transaction between selection and the tip read. That race already existed before this PR. Moving selection inside the hub's generate_lock would leave one lock and fix the root cause. If that is out of scope, please at least correct the comment and changelog wording. The lock does fix the generate-vs-generate case: concurrent_generate_mines_each_pool_tx_once passed for me.
| /// Graph fields come from the cluster, not stub 1/0. Local sendraw is | ||
| /// unbroadcast until a peer getdata completes. | ||
| /// Core `bip125-replaceable` on each mempool entry. | ||
| fn assert_bip125(entries: &[&Value], want: bool) { | ||
| for e in entries { | ||
| assert_eq!(e["bip125-replaceable"], want, "{e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Test doc comment now attaches to the assert_bip125 helper
assert_bip125 and verbose_child were inserted between the doc comment of mempool_graph_fields_follow_cluster_and_unbroadcast ("Graph fields come from the cluster...") and the test itself. That comment now documents the helper, and the test has none.
| pub fn signals_bip125(mp: &MempoolHub, txid: &Txid) -> bool { | ||
| let mut seen = std::collections::HashSet::new(); | ||
| let mut stack = vec![*txid]; | ||
| while let Some(t) = stack.pop() { | ||
| if !seen.insert(t) { | ||
| continue; | ||
| } | ||
| let Some(tx) = mp.get_tx(&t) else { | ||
| continue; | ||
| }; | ||
| if tx.input.iter().any(|i| i.sequence.is_rbf()) { | ||
| return true; | ||
| } | ||
| if let Some((depends, _)) = mp.depends_spentby(&t) { | ||
| stack.extend(depends); | ||
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
nit: signals_bip125 clones every ancestor body
For each ancestor visited, the walk calls MempoolHub::get_tx, which takes a read lock and clones the whole transaction, then calls depends_spentby, which takes a second lock. Verbose getrawmempool does this for every entry. The walk is bounded by the 64-tx cluster cap, so this is cost, not a hazard. A single read guard over the graph that checks input sequences without cloning would avoid it.
Summary
RPC answers that Core clients rely on, found by running Bark's captaind and bark integration suite against this node with no Esplora or electrs (#1165). No relay or consensus policy changes.
--rpc-client-version N(confrpc_client_version=). Defaultgetnetworkinfo.versionstays190000; captaind refuses to boot below310000, so an operator running it sets--rpc-client-version=310000. Only the integer changes. (Was rpc: let operators raise getnetworkinfo.version #1159.)gettxouthides an output of a mempool transaction that another mempool transaction spends. The confirmed branch already checkedspends_outpoint; the mempool branch did not. Core's mempool coins view drops it. A wallet that picks coins withgettxout(include_mempool=true)double-spent its own unconfirmed change and gotinsufficient feefrom the replacement check.getblockverbosity 1 sendssize,strippedsize,weightfor a stale block (confirmations: -1). They were only present at verbosity 2 off the active chain. BDK'sbdk_bitcoind_rpc::Emitterreads verbosegetblockwhile it walks a reorg and failed on the missing field.getmempoolentryand verbosegetrawmempoolreportbip125-replaceable: true when the transaction or an unconfirmed ancestor has an input sequence at or below0xfffffffd, as Core v30 still does.bitcoincore-rpc'sGetMempoolEntryResultrequires the field.generate*/generateblockrun one at a time. Each call selected pool transactions and then connected its block with no lock across the two. Two calls at once could both select a transaction; the second block carried one the first had just confirmed and failedbad-txns-BIP30. Core does both undercs_main.Each fix has a regression test that fails on
masterand passes here. Part of #1164 / #1165. The v3 / TRUC / anchor-package changes are #1192.Test plan
cargo testforrbitcoin-consensus(307),rbitcoin-mempool(171),rbitcoin-net(637),rbitcoin-rpc(135),rbitcoin-test(92): all passcargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all -- --checkrpc_regtest_mature_chain_ops(gettxout, stale getblock),mempool_graph_fields_follow_cluster_and_unbroadcast(bip125),concurrent_generate_mines_each_pool_tx_once(BIP30, 3/3 before, 0/3 after)