Skip to content

[draft] Refactor some validation.cpp methods to return BlockValidationState#19

Draft
optout21 wants to merge 290 commits into
masterfrom
2605-validation-state-return
Draft

[draft] Refactor some validation.cpp methods to return BlockValidationState#19
optout21 wants to merge 290 commits into
masterfrom
2605-validation-state-return

Conversation

@optout21

@optout21 optout21 commented Jun 20, 2026

Copy link
Copy Markdown
Owner

Summary. Refactor validation result to be a return value instead of an output parameter in several validation.cpp methods.

Motivation. The benefits of the change are:

  • Exclude the potentially inconsistent case when the bool return value and the returned state are inconsistent
  • Exclude the ambiguity whether the passed in value of state is used or not (not obvious in chained calls)
  • Remove the possibility of unintuitive interaction between subsequent calls with the same state. In case of a validation error, a failure reason is set if the state was valid, but not if it was already invalid.
  • Slightly simpler: It's more evident which is the result; one less parameters.

This has grown out from bitcoin#33856, mentioned in comment here and here.

Details. Many methods follow the scheme where the validation state is returned in an output parameter (BlockValidationState& state), and and additional bool return value indicating success. In success case the convention is that state.IsValid() and the return value are both true. After the change there is only a BlockValidationState return value, which is either success (state.IsValid() == true), or an invalid/error case.

This change is a highly localized refactor, but touching a sensitive file.

Relevant methods called by ProcessNewBlockHeaders and AcceptBlock (directly and indirectly) are touched.

Changes are separated into commits by touched methods, ordered by bottom-to-top in the call hierarchy.

davidgumberg and others added 10 commits June 18, 2026 16:50
@davidgumberg @hodlinator
test: Tighten getblocktxn checks in parallel cb reconstruction test.
25457a3 
Clear the getblocktxn message so we're not checking existing messages,
and check that the hash in the getblocktxn match the cmpctblock being
announced.

Without this tightening of checks, a later commit that ignores
CMPCTBLOCK messages that are unsolicited will succeed these tests while
silently failing in reality.

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
@davidgumberg
refactor: test: Static assert_highbandwidth_states
44f377a 
This is move-only, and allows assert_highbandwidth_states to be used by
other tests.
@davidgumberg
p2p: Drop unsolicited CMPCTBLOCK from non-HB peer
8313591 
Processing unsolicited CMPCTBLOCK's from a peer that has not been marked
high bandwidth is not well-specified behavior in BIP-0152, in fact the
BIP seems to imply that it is not permitted:

"[...] method is not useful for compact blocks because `cmpctblock`
blocks can be sent unsolicitedly in high-bandwidth mode"

See https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#separate-version-for-segregated-witness

This partially mitigates a mempool leak described in
[bitcoin#28272](bitcoin#28272), but that
particular issue will persist for peers that have been selected as high
bandwidth.

This also mitigates potential DoS / bandwidth-wasting / abusive
behavior that is discussed in the comments of bitcoin#28272.
@davidgumberg
test: p2p: Nodes ignore unsolicited CMPCTBLOCK's
e845e26
@davidgumberg
test: (Un)solicited invalid cb -> get disconnected.
92cea63 
Modifies the invalid_cmpctblock_message test to check that both HB peers
sending unsolicited and non-HB peers sending solicited invalid
cmpctblock's get the boot from us. Also refactors the test to make it
less stateful.
@davidgumberg
p2p: make blocksonly nodes ignore CMPCTBLOCK messages
bf9884f 
blocksonly nodes don't benefit from compact blocks, since they don't
have a mempool to aid in reconstruction, so they should not process
CMPCTBLOCK messages.

This is not just belt-and-suspenders, as a blocksonly node will
trivially reveal exactly which transactions in a block are its own in
the GETBLOCKTXN response to a CMPCTBLOCK. Since it will be missing every
transaction in the block, except for its own.

See discussion: bitcoin#28272
@davidgumberg
p2p: Ignore CMPCTBLOCK from peer that hasn't sent SENDCMPCT
9c5dd29 
This commit also changes the default sendcmpct version in the functional
test to `2`, since this is the version that nodes expect, prior to this
commit, nowhere in the functional test framework was the default version
value used:

git grep -P 'msg_sendcmpct\((?![^)]*version\s*=)' HEAD^

`version=2` is a sensible default, since this is the version nodes
currently expect in the SENDCMPCT handshake.
@hodlinator @davidgumberg
qa: Avoid UTXO reuse between test functions
55e3a57
@rkrux @w0xlt
test: introduce ExtendedPrivateKey and ExtendedPublicKey classes
afdb378 
Using these classes allows the developers to dynamically create
xprvs and xpubs so that they don't need to hardcode such long keys
in the tests that most of the times clutter the tests and make them
difficult to update.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
@rkrux
test: add extendedkey.py unit tests by using BIP32 test vectors
d2a03d5
This was referenced Jun 20, 2026
Closed
Closed
@optout21
optout21 force-pushed the 2605-validation-state-return branch from 8963bb6 to d7e6035 Compare June 20, 2026 07:22
@willcl-ark
ci: use warp docker buildkit cache
b552f17 
This was inadvertently broken in bitcoin#35441
@optout21
optout21 force-pushed the 2605-validation-state-return branch from d7e6035 to a3a5c03 Compare June 20, 2026 13:30
hebasto and others added 15 commits June 21, 2026 13:53
@hebasto
Merge bitcoin#35567: depends: latest config.guess & config.sub
c0922f7 
8f03549 depends: latest config.guess & config.sub (fanquake)

Pull request description:

  Pull the latest versions from upstream.

ACKs for top commit:
  hebasto:
    ACK 8f03549, verified using the upstream [repo](https://cgit.git.savannah.gnu.org/cgit/config.git).

Tree-SHA512: 6ea7ce70a7db614d32f0aea47006892766f6abb41e31fef25cadd4423a0f6e42f8500f03ea70a3990c2628ad1b534b7c52c19914289375f87289ca34349075b2
@sedited
Merge bitcoin#35560: lint: Require scripted-diff script to succeed (t…
ea626c2 
…ake 2)

fab2874 lint: Require scripted-diff script to succeed (MarcoFalke)

Pull request description:

  Currently, scripted diffs may silently pass with errors.

  Fix this issue by calling the script from a Bash instance with error checking enabled: `bash -o errexit -o nounset -o pipefail -c "$SCRIPT"`.

  Also, use Bash (not sh) when launching the script itself, because Bash is required anyway.

  Can be tested by running something like this and observing the behavior before and after:

  ```
  git commit --allow-empty -m $'scripted-diff: foo\n\n-BEGIN VERIFY SCRIPT-\n  false;falseasfsafsaf;true;false|cat; echo "${NO_UN_SET}"|cat  \n-END VERIFY SCRIPT-\n' && ./test/lint/commit-script-check.sh HEAD~..HEAD ; echo $?
  ```

  Alternatively, an ancient brittle script can be tested:

  ```
  ./test/lint/commit-script-check.sh fb65dde~..fb65dde

ACKs for top commit:
  hodlinator:
    ACK fab2874
  sedited:
    ACK fab2874

Tree-SHA512: e3e8167e150be45a096d4883057640eb5624456f21b134cfa901fe490e5afb192855e55a752cb6121399314f65db155544a60645a101c39a084a62de4af23298
@l0rinc
test: raise reindex mining RPC timeout
9e6546c 
`feature_reindex.py` mines a long chain before restarting during reindex.
Bump its local RPC timeout to avoid client timeouts in parallel debug-mode runs.
@sedited
Merge bitcoin#35559: scripted-diff: Rename SteadyClockContext to Fake…
27262a2 
…SteadyClock

855a3fe scripted-diff: Rename SteadyClockContext to FakeSteadyClock (Hao Xu)

Pull request description:

  See: bitcoin#35536 (comment)

  `SteadyClockContext` and `FakeNodeClock` are both LimitOne RAII helpers that mock a clock in tests -- the steady clock and the node clock, respectively. Rename the former so the two follow a consistent FakeXClock naming scheme.

ACKs for top commit:
  maflcko:
    lgtm ACK 855a3fe
  w0xlt:
    ACK 855a3fe
  sedited:
    ACK 855a3fe

Tree-SHA512: 72fd8dd3217d19f4b9f587e379a0aea2b7760ca6d61be6c363e7c06757cba40487df8b40cf3d62f9f9256fb4d0373610dd6f62c6e9ef3edde02265fc967a6bdf
@sedited
Merge bitcoin#35465: coins: compact chainstate regularly
08a25a7 
394e473 coins: compact chainstate in background (Lőrinc)
aa021b2 validation: randomly compact chainstate (Lőrinc)
b10889d coins: test chainstate flush baseline (Lőrinc)

Pull request description:

  **Problem:** bitcoin-core/leveldb-subtree#61 disabled read-triggered seek compactions to avoid large chainstate write amplification from random UTXO lookups.
  That avoids repeated read-driven rewrites, but it also removes opportunistic cleanup that previously helped compact old chainstate data.

  After IBD, normal chainstate churn can leave obsolete entries behind until ordinary LevelDB compaction naturally reaches the affected levels, keeping the chainstate database larger than necessary.

  Also, chainstates created by pre-29 nodes can contain thousands of files from the old 2 MiB LevelDB table target.
  After the mmap limit dropped back to 1000 and seek compaction was disabled, continuing from such a chainstate can leave many table reads on the non-mmap path until the database is compacted.

  **Fix:** After each completed post-IBD full chainstate flush, give the chainstate a 1/320 chance to compact.
  With roughly hourly full flushes, this averages about once every two weeks and makes a six-month stretch without compaction about a one-in-a-million event.

  The randomized recurring trigger spreads compactions across nodes and keeps maintenance stateless, without storing last-compaction height or timestamp metadata in the chainstate database.
  Compaction runs on a background thread (`utxocompact`) so validation only schedules the work.

  Partially fixes bitcoin#35298 and bitcoin#35457

ACKs for top commit:
  sipa:
    ACK 394e473
  optout21:
    ACK 394e473
  andrewtoth:
    ACK 394e473
  sedited:
    ACK 394e473

Tree-SHA512: 536a298f86ca7d87a2dd5428354b05b693931c090c7a4afd4d09b95ce53e2bd8319e01c940489a1520d8cb1010a7ea9724d2c18d2e12887bd5ec70cb68c03517
@pinheadmz
test: cover common HTTP attacks and common malformed requests
881d4b6
@pinheadmz @hodlinator
util/string: use string_view in LineReader
0cdbb19 
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
@pinheadmz
util/string: LineReader should only trim \r or \r\n
5aa3629 
The utility can not be opinionated about CR or SP on either end of a
line it reads. That decision is up to the caller and in fact in the
case of HTTP should be allowed in some places and rejected in others.

Replace TrimStringView() with more surgical operation.
@pinheadmz
http: enclose libevent-dependent code in a namespace
89c54ae 
This commit is a no-op to isolate HTTP methods and objects that
depend on libevent. Following commits will add replacement objects
and methods in a new namespace for testing and review before
switching over the server.
@pinheadmz
http: Implement HTTPHeaders class
68b5d28 
see:
https://www.rfc-editor.org/rfc/rfc2616#section-4.2
https://www.rfc-editor.org/rfc/rfc7231#section-5
https://www.rfc-editor.org/rfc/rfc7231#section-7
https://httpwg.org/specs/rfc9111.html#header.field.definitions
@pinheadmz
http: Implement HTTPResponse class
ad50aa4 
HTTP Response message:
https://datatracker.ietf.org/doc/html/rfc1945#section-6

Status line (first line of response):
https://datatracker.ietf.org/doc/html/rfc1945#section-6.1

Status code definitions:
https://datatracker.ietf.org/doc/html/rfc1945#section-9
@pinheadmz
http: Implement HTTPRequest class
9463e98 
HTTP Request message:
https://datatracker.ietf.org/doc/html/rfc1945#section-5

Request Line aka Control Line aka first line:
https://datatracker.ietf.org/doc/html/rfc1945#section-5.1

See message_read_status() in libevent http.c for how
`MORE_DATA_EXPECTED` is handled there
@pinheadmz @vasild
http: Introduce HTTPServer class and implement binding to listening s…
f5bc018 
…ocket

Introduce a new low-level socket managing class `HTTPServer`.

BindAndStartListening() was copied from CConnMan's BindListenPort()
in net.cpp and modernized.

Unit-test it with a new class `SocketTestingSetup` which mocks
`CreateSock()` and will enable mock client I/O in future commits.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
@pinheadmz @vasild
HTTPServer: implement and test AcceptConnection()
5a3aa1a 
AcceptConnection() is mostly copied from CConmann in net.cpp
and then modernized.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
@pinheadmz @vasild
HTTPServer: generate sequential Ids for each newly accepted connection
a85286c 
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
sedited and others added 29 commits July 9, 2026 14:28
@sedited
Merge bitcoin#35689: test: Inline incorrect check in util_tests
81405fc 
cd2a4bc test: Redeclare variable as signed in `util_tests` (rustaceanrob)

Pull request description:

  Assigning `ToIntegral<int64_t>("-1")` to the `optional<uint64_t>` `n` is a silent underflow. `BOOST_CHECK_EQUAL` then promotes `int` to `uint64_t`, which also underflows. The correct check is to do this inline. Part of clean up in bitcoin#35587

  <details>
  <summary>gdb</summary>

  ```
  (gdb) next
  886    n = ToIntegral<int64_t>("-1", 16);
  1: n = {<std::_Optional_base<unsigned long, true, true>> = {<std::_Optional_base_impl<unsigned long, std::_Optional_base<unsigned long, true, true> >> = {<No data fields>}, _M_payload = {<std::_Optional_payload_base<unsigned long>> = {_M_payload = {
            _M_empty = {<No data fields>}, _M_value = 18446744073709551615},
          _M_engaged = true}, <No data fields>}}, <std::_Enable_copy_move<true, true, true, true, std::optional<unsigned long> >> = {<No data fields>}, <No data fields>}
  2: n = {<std::_Optional_base<unsigned long, true, true>> = {<std::_Optional_base_impl<unsigned long, std::_Optional_base<unsigned long, true, true> >> = {<No data fields>}, _M_payload = {<std::_Optional_payload_base<unsigned long>> = {_M_payload = {
            _M_empty = {<No data fields>}, _M_value = 18446744073709551615},
          _M_engaged = true}, <No data fields>}}, <std::_Enable_copy_move<true, true, true, true, std::optional<unsigned long> >> = {<No data fields>}, <No data fields>}
  (gdb) next
  887    BOOST_CHECK_EQUAL(*n, -1);
  1: n = {<std::_Optional_base<unsigned long, true, true>> = {<std::_Optional_base_impl<unsigned long, std::_Optional_base<unsigned long, true, true> >> = {<No data fields>}, _M_payload = {<std::_Optional_payload_base<unsigned long>> = {_M_payload = {
            _M_empty = {<No data fields>}, _M_value = 18446744073709551615},
          _M_engaged = true}, <No data fields>}}, <std::_Enable_copy_move<true, true, true, true, std::optional<unsigned long> >> = {<No data fields>}, <No data fields>}
  2: n = {<std::_Optional_base<unsigned long, true, true>> = {<std::_Optional_base_impl<unsigned long, std::_Optional_base<unsigned long, true, true> >> = {<No data fields>}, _M_payload = {<std::_Optional_payload_base<unsigned long>> = {_M_payload = {
            _M_empty = {<No data fields>}, _M_value = 18446744073709551615},
          _M_engaged = true}, <No data fields>}}, <std::_Enable_copy_move<true, true, true, true, std::optional<unsigned long> >> = {<No data fields>}, <No data fields>}
  ```
  </details>

ACKs for top commit:
  maflcko:
    review ACK cd2a4bc 🎬
  sedited:
    ACK cd2a4bc

Tree-SHA512: bdcbe94c0445b5359c1ce0c25606359c11fc0024ea0ee629e0233109618101d3c0219ed415dfefefe8775331f7084313e9d8993eca119658fa27a93fb5080d8e
@mzumsande @ryanofsky
test: add test for index commits ahead of the last flushed block
13c02b5 
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
@mzumsande @l0rinc
validation: track last flushed block
09c0696 
This will be used to prevent the indexes from flushing their state ahead of
the chainstate.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
@mzumsande
index: Remove return value from Commit()
6573572 
Since it unused in the current code.
@mzumsande @fjahr
index: Don't commit ahead of the flushed chainstate
3679f1e 
Otherwise, if the node has an unclean restart,
indexes with state (coinstatsindex) couldn't reorg to the
last flushed tip and would be corrupted.

Also updates documentation of Commit() -
the locator functionality isn't used, so the previous text was wrong:
We must have the best block in our block index after a restart.

Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
@mzumsande @l0rinc
validation: Don't use m_chain.Tip() in FlushStateToDisk
e9ed898 
In DisconnectBlock(), we can call FlushStateToDisk after updating
the coins but before changing the tip, which is still at the
disconnected block. This means that the ChainStateFlushed
signal would have the wrong block in the locator.

Also remove an outdated comment - the wallet doesn't use
ChainStateFlushed anymore, currently only indexes do.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
@sipa
chainparams: delete my DNS seed
d908063
@sipa
Remove myself as security contact
629df81
@achow101
Merge bitcoin#34897: indexes: Don't commit ahead of the flushed chain…
7d8137c 
…state

e9ed898 validation: Don't use m_chain.Tip() in FlushStateToDisk (Martin Zumsande)
3679f1e index: Don't commit ahead of the flushed chainstate (Martin Zumsande)
6573572 index: Remove return value from Commit() (Martin Zumsande)
09c0696 validation: track last flushed block (Martin Zumsande)
13c02b5 test: add test for index commits ahead of the last flushed block (Martin Zumsande)

Pull request description:

  If indexes commit their data ahead of the flushed chainstate, and there is an unclean shutdown, the index will be corrupted. This is especially the case for the coinstatsindex, which has state (the muhash) which can't easily be rolled back without access to the blocks. This was only partly fixed in bitcoin#33212 (for reorg scenarios) but could still happen during initial sync.

  Fix this more thoroughly by having the node keep track of the last flushed block, and skipping index commits if the current block of the index is not an ancestor of the node's last flushed block (similar to the suggestion by stickies-v in bitcoin#33212 (review).

  Fixes bitcoin#33208
  Fixes bitcoin#34261

ACKs for top commit:
  achow101:
    ACK e9ed898
  sedited:
    Re-ACK e9ed898
  fjahr:
    re-ACK e9ed898

Tree-SHA512: 7f4dc6fb942d6726587eb75dece24c79c679d8630320502aca9fa2d2b03b1d25999cd11255ec20344ebbb8985552747e2554e2557b9d2ad0c75db71652d615ab
@fanquake
Merge bitcoin#35691: chainparams: delete my DNS seed
e3d67a5 
d908063 chainparams: delete my DNS seed (Pieter Wuille)

Pull request description:

  I plan to shut down my DNS seed server in the future (probably after the release of 32.0), so I request it be removed from the master branch already. It has existed for almost 15 years at this point, and isn't getting much maintenance attention from me anymore.

ACKs for top commit:
  l0rinc:
    ACK d908063
  Sjors:
    ACK d908063
  pinheadmz:
    ACK d908063
  janb84:
    ACK d908063

Tree-SHA512: b3f5f0361d00557b94987a36a8557cf5c6f0ad1415d5ae6b70d64bce55f45f0a3d4c137b9509270128b1ef97b5ef0a3c67e4bbe6c669d5e4a79bd787cc7801c7
@fanquake
Merge bitcoin#35695: Remove myself as security contact
e314869 
629df81 Remove myself as security contact (Pieter Wuille)

Pull request description:

  Making the same change here as in bitcoin-core/bitcoincore.org#1264. I remain involved in security discussions, but don't feel like functioning as a first-line contact anymore.

ACKs for top commit:
  achow101:
    ACK 629df81
  fanquake:
    ACK 629df81

Tree-SHA512: b76813b7e9679cee60a9dd1e8d8de46cd8e329314c677fd4834bde415f8b1f351ebee4eb4c885e889b8a111e8d46d2d20af6d5a4ff108c64df29874511318c2f
@fanquake
Merge bitcoin#35412: ci: add NetBSD Clang cross job
297fd14 
c43b7a1 ci: add netBSD cross CI job (fanquake)
699c21a depends: add netbsd_LDFLAGS (fanquake)

Pull request description:

  This adds a Linux cross job for NetBSD; similar to bitcoin#34491 (FreeBSD).

  This uses the upcoming NetBSD 11.0 release (rc5).

ACKs for top commit:
  willcl-ark:
    ACK c43b7a1

Tree-SHA512: 717b5c68e9cf4dd2b6f9101320ebe94df5047745445c2698005d0dfa034776c2fa836b599155c7cb7e93024b15ba7b91cef07275185cd1c07f58fea678e2354d
@fanquake
doc: archive release notes for v29.4
9b2b3f4
@sedited
Merge bitcoin#35700: doc: archive release notes for v29.4
930f250 
9b2b3f4 doc: archive release notes for v29.4 (fanquake)

Pull request description:

  Archive the release notes for `29.4`.

ACKs for top commit:
  sedited:
    ACK 9b2b3f4

Tree-SHA512: 02cd055a67e68d03613b2e41fb23fe7989296c31bdf76d025eabaa25e65d4c6e6bef42ab7de388c36b9faf519acf754255e507ffbd0585f2cbb4737ddcbd8e6f
@fanquake
Merge bitcoin#35684: Update libmultiprocess subtree to add `max_conne…
b6becf3 
…ctions` option

707d0de Squashed 'src/ipc/libmultiprocess/' changes from 16bf05dea02..28e056576a3 (Ryan Ofsky)

Pull request description:

  The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh).

  Change since last subtree update (bitcoin#35661):

  - Adds an optional `max_connections` parameter to `ListenConnections` ([bitcoin#269](bitcoin-core/libmultiprocess#269))

  This is needed for bitcoin#35037 which lets the maximum number of IPC of incoming connections be configured in bitcoin core.

ACKs for top commit:
  sedited:
    ACK a9d1b65

Tree-SHA512: 1c3ec5c4eb98717c7414a32a3faf63e551b402f7318146745d840fb0e80cbd9e5006892476eb0869eb659cc551ac26d7a3fb2e43509883fb276ac0639a2a7c79
@achow101
Merge bitcoin#35655: wallet: Use in-memory SQLite for temporary walle…
ef101b0 
…t in exportwatchonlywallet

777d23f test: add regression test for in-memory SQLiteDatabase reopen (Pablo Martin)
d1e7f8c wallet: use in-memory SQLite for temporary wallet in exportwatchonlywallet (Pablo Martin)
ee43743 wallet: store m_additional_flags in SQLiteDatabase to fix reopen path (Pablo Martin)

Pull request description:

  Since bitcoin#33032 landed (in-memory `SQLiteDatabase` via `SQLITE_OPEN_MEMORY`), the intermediate wallet built during `exportwatchonlywallet` can live entirely in memory instead of being written to the wallets directory as a temporary file.

  The temp wallet is a pure build artifact: it is populated with descriptors, transactions, and address book data, then immediately discarded once `BackupWallet()` copies its contents to the destination file. Making it in-memory removes all on-disk footprint and eliminates the `cleanup_watchonly_wallet` RAII handler — along with the `wallet_path` and `cleanup_files` variables it needed — which previously ensured the temp files were deleted on both success and failure paths.

  This PR introduces `InMemoryWalletDatabase` (a minimal `SQLiteDatabase` subclass) and `MakeInMemoryWalletDatabase()` factory in `sqlite.h/cpp`, following the same pattern as `MockableSQLiteDatabase` / `CreateMockableWalletDatabase()`. `MockableSQLiteDatabase` now derives from `InMemoryWalletDatabase`, removing its redundant `Files()` override.

  Suggested by Sjors in bitcoin#32489 ([comment](bitcoin#32489 (comment))).

  ---
  Also fixes a related issue found (by Sjors) during review:

  - `SQLiteDatabase::Open()` (the no-arg public override) hardcoded 0 as `additional_flags` when reopening after a failed `TxnAbort()`, which would reopen an in-memory database as on-disk. Fixed by storing `m_additional_flags` in the constructor and using it in the reopen path. For in-memory databases, both the `force_conn_refresh` path and the public `Open()` now throw instead of silently creating a fresh empty connection. A regression test for the `Open()` throw is included in a separate commit.

  ---
  As a follow-up, `InMemoryWalletDatabase` could replace `MockableSQLiteDatabase` in `src/bench/` (5 files, 6 call sites), since benchmarks don't need mock-specific behaviour and benefit from using the same in-memory path as production code.

ACKs for top commit:
  Sjors:
    re-utACK 777d23f
  achow101:
    ACK 777d23f
  janb84:
    ACK 777d23f

Tree-SHA512: 71178ce99c7ebc0fc5ba17956c27d37f90e3d36cefbdc0d15d1424b7a70bf15f05a13cb03c268d885678aba1fd3c90567d8389d3680650c8ae46f4cb4b10b28a
@optout21
Add factory method to BlockValidationState for easier construction
fe3c16d 
Add `InvalidState` static helper to create invalid `BlockValidationState` instance,
for easier construction in error branches. Instead of the 3 statements of
declaration, setting, and return with the exisiting non-static method, now
return&construction is possible in one statement.
@optout21
Refactor CheckBlockHeader signature
20b873e 
Change the (internal) method `CheckBlockHeader` to return the validation
result in the return value instead of an output parameter.
@optout21
Refactor ContextualCheckBlockHeader signature
4b1de61 
Change the (internal) method `ContextualCheckBlockHeader` to return the validation
result in the return value instead of an output parameter.
@optout21
Refactor AcceptBlockHeader signature
0d4eac9 
Change the method `AcceptBlockHeader` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.
@yuvicc @stringintech
@stickies-v @optout21
Change ProcessNewBlockHeaders
ce49352 
Return BlockValidationState by value instead of using an out-parameter,
similar to the TestBlockValidity refactoring in 74690f4.

Remove redundant int return from btck_chainstate_manager_process_block_header.
Previously returned both an int result and an output validation state parameter, creating ambiguity
where non-zero could mean either invalid header or processing failure. Since ProcessNewBlockHeaders already provides complete validation info, the int return was redundant.

Co-authored-by: stringintech <stringintech@gmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
@optout21
Refactor CheckMerkleRoot signature
52204bc 
Change the (internal) method `CheckMerkleRoot` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.
@optout21
Refactor CheckBlock signature
e333011 
Change the method `CheckBlock` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.
@optout21
Refactor CheckWitnessMalleation signature
36ad0af 
Change the (internal) method `CheckWitnessMalleation` to return the
`BlockValidationState` validation result in the return value
instead of an output parameter.
@optout21
Refactor ContextualCheckBlock signature
8aa5f52 
Change the (internal) method `ContextualCheckBlock` to return the
`BlockValidationState` validation result in the return value
instead of an output parameter.
@optout21
Refactor FatalError signature
020f4ba 
Change the method `FatalError` to return the constructed
`BlockValidationState` object in a return value instead of
an output parameter.
@optout21
Refactor FlushStateToDisk signature
cd326cd 
Change the method `FlushStateToDisk` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.
@optout21
Refactor AcceptBlock signature
3764f59 
Change the method `AcceptBlock` to return the `BlockValidationState`
validation result in the return value instead of an output parameter.
@optout21
Internal simplification in TestBlockValidity
e600c4c
@optout21
optout21 force-pushed the 2605-validation-state-return branch from 6e5a039 to e600c4c Compare July 12, 2026 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.