Address bitcoind RPC review: gettxout/getrawtransaction fixes, compatibility nits, and refactors#822
Open
dsbaars wants to merge 4 commits into
Open
Address bitcoind RPC review: gettxout/getrawtransaction fixes, compatibility nits, and refactors#822dsbaars wants to merge 4 commits into
dsbaars wants to merge 4 commits into
Conversation
- gettxout: use is_confirmed_spent instead of is_spent to match bitcoind gettxout semantics (mempool ignored = confirmed UTXO set only). is_spent also counts unconfirmed/conflicting/invalid-block spenders, which would wrongly return null for outputs bitcoind still reports as unspent. - getrawtransaction: validate verbose via to_integer (as get_block does) rather than a raw double compare, matching bitcoind ParseVerbosity. - getblockchaininfo: clarify blocks/headers are heights (not counts) per bitcoind convention; no functional change.
- getblockchaininfo/getblockheader/getblock: add chainwork field via query.get_work, matching bitcoind; add target to getblockheader. - inject_block_context: report -1 confirmations for a block not on the active chain, as bitcoind does (was unconditionally positive). - getblockfilter: reject filter types other than "basic" (neutrino), as bitcoind validates filtertype before checking index availability. - Fix "in in v4" comment typo. Verified against the bitcoind unit test suite (51 cases, 421 assertions).
- getblock: replace magic verbosity levels (0/1/2) with a block_verbosity enum, dedupe the block fetch, and reject out-of-range verbosity. - getblockchaininfo: extract the verificationprogress computation into a verification_progress utility method. - getnetworkinfo: source subversion, protocolversion and localrelay from network configuration (user_agent, protocol_maximum, enable_relay) instead of hardcoded literals. Verified against the bitcoind unit test suite (51 cases, 421 assertions).
Link the node-utility/tx_chaser TODOs to libbitcoin-node#1075 and the signet chain selector TODO to libbitcoin-system#1908.
evoskuil
reviewed
Jul 9, 2026
| namespace { | ||
| enum class block_verbosity : size_t | ||
| { | ||
| hex = 0, // serialized block, hex-encoded |
Member
There was a problem hiding this comment.
Style, generally avoid trailing comments.
|
|
||
| // bitcoind reports -1 confirmations for a block not on the active chain. | ||
| out["confirmations"] = confirmed ? | ||
| static_cast<int64_t>(add1(floored_subtract(top, height))) : -1; |
| size_t headers) NOEXCEPT | ||
| { | ||
| return is_zero(headers) ? 1.0 : | ||
| std::min(1.0, static_cast<double>(blocks) / |
| { | ||
| return is_zero(headers) ? 1.0 : | ||
| std::min(1.0, static_cast<double>(blocks) / | ||
| static_cast<double>(headers)); |
| } | ||
|
|
||
| if (level == one || level == two) | ||
| const auto detail = static_cast<block_verbosity>(level); |
Member
There was a problem hiding this comment.
Converting to integral and then casting for each usage isn't making for the most readable code. I would recommend providing to/from methods for the enum that provide an invalid value upon float conversion, internally handles invalid float, and then exclusively use the enum values.
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.
Summary
This branch addresses @evoskuil's review of the bitcoind-compatible JSON-RPC implementation. It resolves the open review questions, fixes bitcoind compatibility nits found while verifying against Bitcoin Core, completes the self-contained refactor TODOs, and links the remaining upstream-blocked TODOs to tracking issues. Bitcoin Core semantics were checked against the reference implementation in bitcoin/bitcoin.
Review questions
is_confirmed_spentinstead ofis_spent. With mempool ignored this matches bitcoind gettxout (include_mempool=false, the confirmed UTXO set).is_spentalso counts unconfirmed, conflicting, and invalid-block spenders, which would wrongly return null for outputs bitcoind still reports as unspent.to_integer(as getblock does) instead of a raw double compare, matching bitcoind ParseVerbosity.Compatibility nits
query.get_work. bitcoind reports it and it was missing.Refactors (review TODOs)
block_verbosityenum and deduped the block fetch.verification_progressutility.user_agent,protocol_maximum,enable_relay) instead of hardcoded literals.Upstream-blocked TODOs (linked, not changed)
Some review TODOs require upstream changes and are now referenced from the code:
get_pool_context/validate_tx/broadcast_txduplication and thenode::tx_chaserpath): Provide a node-side transaction validation utility (dedup pool-context/validate_tx across server protocols) libbitcoin-node#1075.The only remaining local TODO is precomputing response size hints, a minor optimization left as-is.
Verification
Built the full dependency stack (system, database, network, node) statically in Release and compiled and linked this branch against it. The bitcoind unit test suite passes: 51 cases, 421 assertions, no failures. The gettxout and confirmations changes were checked against the existing tests and against bitcoin/bitcoin.