Paths

Table of Contentst

Differential D20593

follow-ups for core#29612
ClosedPublic
Actions

Authored by PiRK on Tue, Sep 8, 15:29.

Project Tags

None

Referenced Files

	F86007176: D20593.1791416145.diff
	Tue, Oct 6, 23:35

	F86007174: D20593.1791416143.diff
	Tue, Oct 6, 23:35

	F86007173: D20593.1791416142.diff
	Tue, Oct 6, 23:35

	F86007171: D20593.1791416103.diff
	Tue, Oct 6, 23:35

	F86001917: D20593.1791392175.diff
	Tue, Oct 6, 16:56

Subscribers

Fabien

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC70ba2632c51d: follow-ups for core#29612

Summary

assumeutxo: Deserialize trailing byte instead of Txid

From the core#29612 review:

nit: I think this should check that the stream is completely empty, no? I suspect that a trailing byte will be left undetected, so it may be better to deserialize a std::byte here?

https://github.com/bitcoin/bitcoin/pull/30167/changes/1f1f9984555d49f07ae20cb3a8153a177c546beb

assumeutxo: Add network magic ctor param to SnapshotMetadata

This prevents SnapshotMetadata from using any globals implicitly.

test: Add coverage for txid coins count check when loading snapshot

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Tue, Sep 8, 15:29

Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Sep 8, 15:29

PiRK requested review of this revision.Tue, Sep 8, 15:29

teamcity edited the summary of this revision. (Show Details)Tue, Sep 8, 15:29

Harbormaster completed remote builds in B38110: Diff 61257.Tue, Sep 8, 15:33

Build Bitcoin ABC Diffs / Diff Testing (build-fuzzer) failed.

Tail of the build log:

[379/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_hd_keypath.cpp.o
[380/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_parsing.cpp.o
[381/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/pow.cpp.o
[382/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/net.cpp.o
[383/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/primitives_transaction.cpp.o
[384/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/protocol.cpp.o
[385/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/random.cpp.o
[386/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/signature_checker.cpp.o
[387/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/rolling_bloom_filter.cpp.o
[388/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_descriptor_cache.cpp.o
[389/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_bitcoin_consensus.cpp.o
[390/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_ops.cpp.o
[391/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script.cpp.o
[392/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_interpreter.cpp.o
[393/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_message.cpp.o
[394/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_in.cpp.o
[395/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_messages.cpp.o
[396/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_out.cpp.o
[397/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/scriptnum_ops.cpp.o
[398/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ecdsa_signature_parse_der_lax.cpp.o
[399/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[400/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[401/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[402/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[403/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[404/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[405/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ec_seckey_import_export_der.cpp.o
[406/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sigcache.cpp.o
[407/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[408/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[409/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/span.cpp.o
[410/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sign.cpp.o
[411/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[412/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[413/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[414/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o
[415/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o
[416/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/socks5.cpp.o
[417/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[418/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[419/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/string.cpp.o
[420/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[421/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[422/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o
[423/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/util.cpp.o
[424/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/transaction.cpp.o
[425/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/txrequest.cpp.o
[426/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o
[427/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[428/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[429/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[430/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[431/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[432/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[433/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[434/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[435/437] Linking CXX static library src/wallet/libwallet.a
[436/437] Linking CXX static library src/libserver.a
ninja: build stopped: cannot make progress due to previous errors.
Build build-fuzzer failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) Your tests are like unicorns: frequently referenced, rarely seen.

────────────────────────────────────────────────────────────────────────

minor [Functional Correctness]
→ ]8;;vscode://file//work/src/test/fuzz/deserialize.cpp:325src/test/fuzz/deserialize.cpp:325-326]8;;

Use disk magic for SnapshotMetadata.

SnapshotMetadata::Serialize() and loadtxoutset use DiskMagic().
MessageStart() is the network message magic. The values differ,
including on regtest. A production-format snapshot therefore takes the
network-mismatch path in this fuzzer instead of the accepted metadata
path. Construct SnapshotMetadata with Params().DiskMagic().
(github.com)


Proposed fix

-    auto msg_start = Params().MessageStart();
-    SnapshotMetadata snapshot_metadata{msg_start};
+    SnapshotMetadata snapshot_metadata{Params().DiskMagic()};

────────────────────────────────────────
Review complete
1 finding ✔

Minor 1

8 files reviewed:

src/kernel/chainparams.cpp
src/kernel/chainparams.h
src/node/utxo_snapshot.h
src/rpc/blockchain.cpp
src/test/fuzz/deserialize.cpp
src/test/util/chainstate.h
src/validation.cpp
test/functional/feature_assumeutxo.py

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

[41/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/fees.cpp.o
[42/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/flatfile.cpp.o
[43/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/key_io.cpp.o
[44/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/http_request.cpp.o
[45/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/golomb_rice.cpp.o
[46/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/key.cpp.o
[47/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/overflow.cpp.o
[48/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/kitchen_sink.cpp.o
[49/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/integer.cpp.o
[50/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_iso8601.cpp.o
[51/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/merkleblock.cpp.o
[52/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/message.cpp.o
[53/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_numbers.cpp.o
[54/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_script.cpp.o
[55/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/multiplication_overflow.cpp.o
[56/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/muhash.cpp.o
[57/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/net_permissions.cpp.o
[58/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/prevector.cpp.o
[59/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/netaddress.cpp.o
[60/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/p2p_transport_deserializer.cpp.o
[61/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/load_external_block_file.cpp.o
[62/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/node_eviction.cpp.o
[63/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/psbt.cpp.o
[64/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_hd_keypath.cpp.o
[65/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_univalue.cpp.o
[66/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/net.cpp.o
[67/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_flags.cpp.o
[68/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/pow.cpp.o
[69/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_parsing.cpp.o
[70/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/protocol.cpp.o
[71/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/primitives_transaction.cpp.o
[72/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/random.cpp.o
[73/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_bitcoin_consensus.cpp.o
[74/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/rolling_bloom_filter.cpp.o
[75/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script.cpp.o
[76/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_descriptor_cache.cpp.o
[77/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/signature_checker.cpp.o
[78/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_messages.cpp.o
[79/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_message.cpp.o
[80/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_interpreter.cpp.o
[81/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_ops.cpp.o
[82/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/scriptnum_ops.cpp.o
[83/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ec_seckey_import_export_der.cpp.o
[84/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ecdsa_signature_parse_der_lax.cpp.o
[85/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_in.cpp.o
[86/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_out.cpp.o
[87/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sign.cpp.o
[88/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/span.cpp.o
[89/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sigcache.cpp.o
[90/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o
[91/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o
[92/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o
[93/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/string.cpp.o
[94/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/socks5.cpp.o
[95/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/transaction.cpp.o
[96/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/util.cpp.o
[97/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/txrequest.cpp.o
[98/99] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1