Page MenuHomePhabricator
Differential D11453

[avalanche] Introduce a CompactProofs class for managing the short proof ids
ClosedPublic
Actions

Authored by Fabien on May 12 2022, 12:55.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCe0c88574ae1e: [avalanche] Introduce a CompactProofs class for managing the short proof ids
Summary

This class serializes and deserializes the proof short ids. This will be used to send and process the compact proof messages.

Depends on D11450.

Test Plan
ninja check-avalanche

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien created this revision.May 12 2022, 12:55
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 12 2022, 12:55
Fabien requested review of this revision.May 12 2022, 12:55
Harbormaster completed remote builds in B19010: Diff 33495.May 12 2022, 13:11
deadalnix requested changes to this revision.May 12 2022, 19:22
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/compactproofs.h
53 ↗(On Diff #33495)

What's the reason for this? If we hit that branch, we just deserialized 16GB of data, as far as I can tell, so if this is an anti DoS measure, this would be better if it failed BEFORE that. If this serves another purpose, then what?

This revision now requires changes to proceed.May 12 2022, 19:22
Fabien updated this revision to Diff 33500.May 12 2022, 20:06

Remove useless and unreachable safety net

Fabien planned changes to this revision.May 12 2022, 20:12

This will need to be rebased

Harbormaster completed remote builds in B19014: Diff 33500.May 12 2022, 20:23
Fabien updated this revision to Diff 33541.May 16 2022, 12:01

Rebase

Harbormaster completed remote builds in B19047: Diff 33541.May 16 2022, 12:17
Fabien updated this revision to Diff 33551.May 17 2022, 08:56

Take a shared_ptr

Harbormaster completed remote builds in B19053: Diff 33551.May 17 2022, 09:13
Fabien added a child revision: D11388: [avalanche] Answer getavaproofs message with short proof ids.May 17 2022, 09:21
deadalnix requested changes to this revision.May 20 2022, 20:56
deadalnix added inline comments.
src/avalanche/compactproofs.h
31 ↗(On Diff #33551)

IMO you want to follow the same format as compact block and add the capability to add proofs inline, even if it is always an empty vector initially.

This revision now requires changes to proceed.May 20 2022, 20:56
Fabien updated this revision to Diff 33664.May 23 2022, 11:11

Add the ability to include prefilled proofs inline

Fabien edited the summary of this revision. (Show Details)May 23 2022, 11:11
Fabien added a parent revision: D11514: [RCU] Make it possible to serialize/deserialize a RCUPtr.
Harbormaster completed remote builds in B19135: Diff 33664.May 23 2022, 11:26
deadalnix requested changes to this revision.May 23 2022, 12:27
deadalnix added inline comments.
src/avalanche/compactproofs.h
68 ↗(On Diff #33664)

But now you need to check the 32 bit overflow condition.

This revision now requires changes to proceed.May 23 2022, 12:27
Fabien added inline comments.May 23 2022, 18:54
src/avalanche/compactproofs.h
68 ↗(On Diff #33664)

I think the overflow check is not doing what you expect, and it is unreachable in the compact block code as well.
It is checking the vector sizes is < max(uint32_t) but not checking the indexes are overflowing. This check is done by the DifferenceFormatterfor the txs request and when initializing the block from the compact block data (`PartiallyDownloadedBlock::InitData'). Note that this actually works because the coinbase is the only prefilled transaction, and so the index is hardcoded to zero, so it works during serialization without the differential encoder.

For the proofs I think we can make it correct from the beginning, and use the formatter for the prefilled proofs.

Fabien updated this revision to Diff 33677.May 23 2022, 18:55

Use a Differential formatter for serializing/deserializing the prefilled proofs.
Add unit tests for the various possible overflow cases.

Fabien edited the summary of this revision. (Show Details)May 23 2022, 18:56
Fabien added a parent revision: D11515: Move the DifferenceFormatter to serialize.h.
Fabien removed a parent revision: D11514: [RCU] Make it possible to serialize/deserialize a RCUPtr.
Harbormaster completed remote builds in B19142: Diff 33677.May 23 2022, 19:21
Fabien planned changes to this revision.May 24 2022, 14:21

Needs rebase

Fabien updated this revision to Diff 33685.May 24 2022, 14:29

Rebase

Fabien edited the summary of this revision. (Show Details)May 24 2022, 14:29
Fabien removed a parent revision: D11515: Move the DifferenceFormatter to serialize.h.
Harbormaster completed remote builds in B19147: Diff 33685.May 24 2022, 14:44
deadalnix added inline comments.May 24 2022, 15:53
src/avalanche/compactproofs.h
68 ↗(On Diff #33664)

The reason the check cannot be triggered if because there is a limit in the vector deserialization code.

Fabien added inline comments.May 24 2022, 16:46
src/avalanche/compactproofs.h
68 ↗(On Diff #33664)

That's not enough, you can still trigger an index overflow (not a size overflow, which only happens if you successfully unserialized GB of data) without triggering the vector deserialization limit because the indexes are encoded as a difference from the previous index in the vector: the difference formatter will catch these cases (see the unit test).

Fabien updated this revision to Diff 33717.May 30 2022, 13:06

Rebase

Harbormaster completed remote builds in B19174: Diff 33717.May 30 2022, 13:10
teamcity added a comment.May 30 2022, 13:10
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

[348/520] Linking C static library src/secp256k1/libsecp256k1.a
[349/520] Linking CXX static library src/libbitcoinconsensus.a
[350/520] Linking C executable src/secp256k1/ecmult-bench
[351/520] Linking C executable src/secp256k1/internal-bench
[352/520] Linking C executable src/secp256k1/sign-bench
[353/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[354/520] Linking C executable src/secp256k1/verify-bench
[355/520] Linking C executable src/secp256k1/recover-bench
[356/520] Installing component secp256k1
-- Install configuration: "RelWithDebInfo"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[357/520] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[358/520] Linking CXX static library src/libscript.a
[359/520] Linking CXX static library src/libcommon.a
[360/520] Linking CXX shared library src/libbitcoinconsensus.so.0.25.6
[361/520] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[362/520] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[363/520] Linking CXX executable src/bitcoin-cli
[364/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[365/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[366/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[367/520] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[368/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[369/520] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[370/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[371/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[372/520] Linking CXX executable src/bitcoin-tx
[373/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[374/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[375/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[376/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[377/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[378/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[379/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[380/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[381/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[382/520] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[383/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[384/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[385/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[386/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[387/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[388/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[389/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[390/520] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[391/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[392/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[393/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[394/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[395/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[396/520] Linking CXX static library src/wallet/libwallet.a
[397/520] Linking CXX static library src/wallet/libwallet-tool.a
[398/520] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
teamcity added a comment.May 30 2022, 13:13
Build Bitcoin ABC Diffs / Diff Testing (build-clang) failed.

Tail of the build log:

[348/520] Installing component secp256k1
-- Install configuration: "RelWithDebInfo"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[349/520] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[350/520] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[351/520] Linking CXX static library src/libbitcoinconsensus.a
[352/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[353/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[354/520] Building CXX object src/CMakeFiles/script.dir/script/standard.cpp.o
[355/520] Linking CXX static library src/libscript.a
[356/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[357/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[358/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[359/520] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[360/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[361/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[362/520] Linking C executable src/secp256k1/ecmult-bench
[363/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[364/520] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[365/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[366/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[367/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[368/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[369/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[370/520] Building CXX object src/CMakeFiles/common.dir/rpc/util.cpp.o
[371/520] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[372/520] Linking CXX static library src/libcommon.a
[373/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[374/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[375/520] Linking CXX shared library src/libbitcoinconsensus.so.0.25.6
[376/520] Linking CXX executable src/bitcoin-cli
[377/520] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[378/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[379/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[380/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[381/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[382/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[383/520] Building CXX object src/CMakeFiles/server.dir/net_processing.cpp.o
[384/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[385/520] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[386/520] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[387/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[388/520] Linking CXX executable src/bitcoin-tx
[389/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[390/520] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[391/520] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[392/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[393/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[394/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[395/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[396/520] Linking CXX static library src/wallet/libwallet.a
[397/520] Linking CXX static library src/wallet/libwallet-tool.a
[398/520] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1
teamcity added a comment.May 30 2022, 13:15
Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

[340/518] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[341/518] Building CXX object src/CMakeFiles/common.dir/rpc/util.cpp.o
[342/518] Building CXX object src/CMakeFiles/script.dir/script/signingprovider.cpp.o
[343/518] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[344/518] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[345/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[346/518] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[347/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[348/518] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[349/518] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[350/518] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[351/518] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[352/518] Linking C static library src/secp256k1/libsecp256k1.a
[353/518] Linking CXX static library src/libbitcoinconsensus.a
[354/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[355/518] Linking CXX static library src/libscript.a
[356/518] Linking C executable src/secp256k1/ecmult-bench
[357/518] Linking CXX static library src/libcommon.a
[358/518] Linking C executable src/secp256k1/internal-bench
[359/518] Linking CXX shared library src/libbitcoinconsensus.so.0.25.6
[360/518] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[361/518] Linking CXX executable src/bitcoin-cli
[362/518] Linking C executable src/secp256k1/sign-bench
[363/518] Linking C executable src/secp256k1/verify-bench
[364/518] Linking C executable src/secp256k1/recover-bench
[365/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[366/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[367/518] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[368/518] Linking CXX executable src/bitcoin-tx
[369/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[370/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[371/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[372/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[373/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[374/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[375/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[376/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[377/518] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[378/518] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[379/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[380/518] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[381/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[382/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[383/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[384/518] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[385/518] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[386/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[387/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[388/518] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[389/518] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[390/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[391/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[392/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[393/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[394/518] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[395/518] Linking CXX static library src/wallet/libwallet.a
[396/518] Linking CXX static library src/wallet/libwallet-tool.a
[397/518] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
teamcity added a comment.May 30 2022, 13:15
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

[341/466] Building CXX object src/CMakeFiles/server.dir/torcontrol.cpp.o
[342/466] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[343/466] Building CXX object src/CMakeFiles/server.dir/dummywallet.cpp.o
[344/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[345/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[346/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[347/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[348/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[349/466] Linking C static library src/secp256k1/libsecp256k1.a
[350/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[351/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[352/466] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[353/466] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[354/466] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[355/466] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[356/466] Linking C executable src/secp256k1/ecmult-bench
[357/466] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[358/466] Linking C executable src/secp256k1/internal-bench
[359/466] Linking C executable src/secp256k1/sign-bench
[360/466] Linking C executable src/secp256k1/verify-bench
[361/466] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[362/466] Linking C executable src/secp256k1/recover-bench
[363/466] Linking CXX static library src/libcommon.a
[364/466] Linking CXX static library src/libscript.a
[365/466] Linking CXX static library src/libbitcoinconsensus.a
[366/466] Linking CXX shared library src/libbitcoinconsensus.so.0.25.6
[367/466] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[368/466] Linking CXX executable src/bitcoin-cli
[369/466] Building CXX object src/CMakeFiles/server.dir/avalanche/compactproofs.cpp.o
FAILED: src/CMakeFiles/server.dir/avalanche/compactproofs.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DBOOST_AC_USE_STD_ATOMIC -DBOOST_SP_USE_STD_ATOMIC -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/leveldb/helpers/memenv -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-reuse=none -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wredundant-decls -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Wformat-security -Woverloaded-virtual -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/avalanche/compactproofs.cpp.o -MF src/CMakeFiles/server.dir/avalanche/compactproofs.cpp.o.d -o src/CMakeFiles/server.dir/avalanche/compactproofs.cpp.o -c ../../src/avalanche/compactproofs.cpp
In file included from ../../src/avalanche/compactproofs.cpp:5:
../../src/./avalanche/compactproofs.h:59:25: error: ‘RadixTree’ does not name a type
     CompactProofs(const RadixTree<const Proof, ProofRadixTreeAdapter> &proofs);
                         ^~~~~~~~~
../../src/./avalanche/compactproofs.h:59:34: error: expected ‘,’ or ‘...’ before ‘<’ token
     CompactProofs(const RadixTree<const Proof, ProofRadixTreeAdapter> &proofs);
                                  ^
../../src/avalanche/compactproofs.cpp:13:11: error: ‘RadixTree’ does not name a type
     const RadixTree<const Proof, ProofRadixTreeAdapter> &proofs)
           ^~~~~~~~~
../../src/avalanche/compactproofs.cpp:13:20: error: expected ‘,’ or ‘...’ before ‘<’ token
     const RadixTree<const Proof, ProofRadixTreeAdapter> &proofs)
                    ^
../../src/avalanche/compactproofs.cpp: In constructor ‘avalanche::CompactProofs::CompactProofs(int)’:
../../src/avalanche/compactproofs.cpp:15:5: error: ‘proofs’ was not declared in this scope
     proofs.forEachLeaf([&](auto pLeaf) {
     ^~~~~~
../../src/avalanche/compactproofs.cpp:15:5: note: suggested alternative: ‘Proof’
     proofs.forEachLeaf([&](auto pLeaf) {
     ^~~~~~
     Proof
[370/466] Linking CXX executable src/bitcoin-tx
[371/466] Building CXX object src/CMakeFiles/server.dir/avalanche/processor.cpp.o
[372/466] Building CXX object src/CMakeFiles/server.dir/rpc/avalanche.cpp.o
[373/466] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[374/466] Building CXX object src/CMakeFiles/server.dir/init.cpp.o
[375/466] Building CXX object src/CMakeFiles/server.dir/net_processing.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1
teamcity added a comment.May 30 2022, 13:17
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

[348/520] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[349/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[350/520] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[351/520] Linking C static library src/secp256k1/libsecp256k1.a
[352/520] Linking CXX static library src/libbitcoinconsensus.a
[353/520] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[354/520] Linking C executable src/secp256k1/ecmult-bench
[355/520] Linking C executable src/secp256k1/internal-bench
[356/520] Linking C executable src/secp256k1/sign-bench
[357/520] Linking CXX static library src/libscript.a
[358/520] Linking C executable src/secp256k1/verify-bench
[359/520] Linking C executable src/secp256k1/recover-bench
[360/520] Installing component secp256k1
-- Install configuration: "Debug"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[361/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[362/520] Linking CXX static library src/libcommon.a
[363/520] Linking CXX shared library src/libbitcoinconsensus.so.0.25.6
[364/520] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[365/520] Linking CXX executable src/bitcoin-cli
[366/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[367/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[368/520] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[369/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[370/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[371/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[372/520] Linking CXX executable src/bitcoin-tx
[373/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[374/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[375/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[376/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[377/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[378/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[379/520] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[380/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[381/520] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[382/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[383/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[384/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[385/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[386/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[387/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[388/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[389/520] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[390/520] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[391/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[392/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[393/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[394/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[395/520] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[396/520] Linking CXX static library src/wallet/libwallet.a
[397/520] Linking CXX static library src/wallet/libwallet-tool.a
[398/520] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1
Fabien updated this revision to Diff 33719.May 30 2022, 13:17

Add missing include

Harbormaster completed remote builds in B19176: Diff 33719.May 30 2022, 13:37
deadalnix added inline comments.May 31 2022, 19:23
src/avalanche/compactproofs.h
31 ↗(On Diff #33719)

I don't understand why the prefilled proofs need to have an index. Proofs are inherently unordered, so this convey no useful information. or does it?

Fabien added inline comments.May 31 2022, 19:47
src/avalanche/compactproofs.h
31 ↗(On Diff #33719)

There is the same thing for the compact block transactions, this lets the receiver know which index to request and which to skip without the caller having to remember about what was sent.
Say you "prefill" a transaction at index 10 in a block of 20 txs, you will have 19 shortids with indexes 0-9 then 11-19 that can be requested. No need for the caller to remember that there was a prefilled transaction at index 10 to send the expected tx. The same applies to the proofs, by using an index with a prefilled proof the caller only needs the radix tree to retrieve the expected proof from a requested index.

deadalnix requested changes to this revision.Jun 1 2022, 11:07
deadalnix added inline comments.
src/avalanche/compactproofs.h
31 ↗(On Diff #33719)

It strikes me as a lot of complication to save 32bits per transaction. But we can do that for consistency with the compact block thing.

That being said, it means that you need ot check for the 32 bits overflow during serialization/deserialization.

This revision now requires changes to proceed.Jun 1 2022, 11:07
Fabien added inline comments.Jun 1 2022, 13:04
src/avalanche/compactproofs.h
31 ↗(On Diff #33719)

the overflow check happens in the DifferenceFormatter

Fabien requested review of this revision.Jun 1 2022, 13:04
deadalnix requested changes to this revision.Jun 1 2022, 15:43
deadalnix added inline comments.
src/avalanche/compactproofs.h
31 ↗(On Diff #33719)

No, this does not check the same thing.

This revision now requires changes to proceed.Jun 1 2022, 15:43
Fabien updated this revision to Diff 33783.Jun 2 2022, 13:22

Fix an off by one error in the test, add a 32 bits overflow check and non contiguous index check at deserialization time

Harbormaster completed remote builds in B19230: Diff 33783.Jun 2 2022, 13:36
Fabien added a child revision: D11568: Extract the PrefilledProof::Formatter so it can be reused for compact blocks.Jun 3 2022, 19:08
deadalnix accepted this revision.Jun 6 2022, 12:30
This revision is now accepted and ready to land.Jun 6 2022, 12:30
Closed by commit rABCe0c88574ae1e: [avalanche] Introduce a CompactProofs class for managing the short proof ids (authored by Fabien). · Explain WhyJun 6 2022, 14:33
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABCe0c88574ae1e: [avalanche] Introduce a CompactProofs class for managing the short proof ids.

Revision Contents
Changeset List

PathSize
src/
1 line
avalanche/
106 lines
28 lines
test/
1 line
318 lines

Diff 33824

View Options

src/CMakeLists.txt

Loading...
View Options

src/avalanche/compactproofs.h

Loading...
View Options

src/avalanche/compactproofs.cpp

Loading...
View Options

src/avalanche/test/CMakeLists.txt

Loading...
View Options

src/avalanche/test/compactproofs_tests.cpp

Loading...