Details

Reviewers

majcosta
deadalnix

Group Reviewers

Restricted Project

Commits

rABC1d95857cbced: Use rolling bloom filter of recent block tx's for AlreadyHave() check

Summary

In order to determine whether to download or process a relayed transaction, we
try to determine if we already have the transaction, either in the mempool, in
our recently rejected filter, in our orphan pool, or already confirmed in the
chain itself.

Prior to this commit, the heuristic for checking the chain is based on whether
there's an output corresponding to the 0- or 1-index vout in our coin cache.
While that is a quick check, it is very imprecise (say if those outputs were
already spent in a block) -- we can do better by just keeping a rolling bloom
filter of the transactions in recent blocks, which will capture the case of a
transaction which has been confirmed and then fully spent already.

To avoid relay problems for transactions which have been included in a recent
block but then reorged out of the chain, we clear the bloom filter whenever a
block is disconnected.

This is a backport of PR17951

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.Dec 18 2020, 11:11

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 18 2020, 11:11

PiRK requested review of this revision.Dec 18 2020, 11:12

teamcity edited the summary of this revision. (Show Details)Dec 18 2020, 11:12

I noticed some difference between Core and ABC: Core uses transaction hashes while ABC uses transaction ids. I don't know if it makes a difference in the behavior. I choose to stick with hashes, like in the original PR

Harbormaster completed remote builds in B14441: Diff 26423.Dec 18 2020, 11:15

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

Tail of the build log:

-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[326/498] Building CXX object src/CMakeFiles/util.dir/util/message.cpp.o
[327/498] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[328/498] Building CXX object src/CMakeFiles/util.dir/util/settings.cpp.o
[329/498] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/primitives/transaction.cpp.o
[330/498] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/util/strencodings.cpp.o
[331/498] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[332/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[333/498] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[334/498] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[335/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[336/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[337/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[338/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[339/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[340/498] Building CXX object src/CMakeFiles/util.dir/util/time.cpp.o
[341/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[342/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[343/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[344/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[345/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[346/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[347/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[348/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[349/498] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o
[350/498] Linking CXX static library src/libutil.a
[351/498] Linking CXX static library src/librpcclient.a
[352/498] Linking CXX static library src/libbitcoinconsensus.a
[353/498] Linking CXX static library src/libscript.a
[354/498] Linking CXX static library src/libcommon.a
[355/498] Linking CXX shared library src/libbitcoinconsensus.so.0.22.10
[356/498] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[357/498] Linking CXX executable src/bitcoin-cli
[358/498] Linking CXX executable src/bitcoin-tx
[359/498] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[360/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[361/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[362/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[363/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[364/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[365/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[366/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[367/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[368/498] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[369/498] Linking CXX static library src/zmq/libzmq.a
[370/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[371/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[372/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[373/498] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[374/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[375/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[376/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[377/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[378/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[379/498] Linking CXX static library src/wallet/libwallet.a
[380/498] Linking CXX static library src/wallet/libwallet-tool.a
[381/498] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

PiRK edited the summary of this revision. (Show Details)Dec 18 2020, 11:16

remove extra blank line

Harbormaster completed remote builds in B14442: Diff 26424.Dec 18 2020, 11:19

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

Tail of the build log:

[341/498] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[342/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[343/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[344/498] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[345/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[346/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[347/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[348/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[349/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[350/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[351/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[352/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[353/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[354/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[355/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[356/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[357/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[358/498] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[359/498] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[360/498] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[361/498] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[362/498] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[363/498] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[364/498] Linking C static library src/secp256k1/libsecp256k1.a
[365/498] Linking CXX static library src/libbitcoinconsensus.a
[366/498] 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
[367/498] Linking C executable src/secp256k1/sign-bench
[368/498] Linking C executable src/secp256k1/internal-bench
[369/498] Linking C executable src/secp256k1/ecmult-bench
[370/498] Linking C executable src/secp256k1/verify-bench
[371/498] Linking C executable src/secp256k1/recover-bench
[372/498] Linking CXX static library src/libscript.a
[373/498] Linking CXX static library src/libcommon.a
[374/498] Linking CXX shared library src/libbitcoinconsensus.so.0.22.10
[375/498] Linking CXX executable src/bitcoin-cli
[376/498] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[377/498] Linking CXX executable src/bitcoin-tx
[378/498] Linking CXX static library src/wallet/libwallet.a
[379/498] Linking CXX static library src/wallet/libwallet-tool.a
[380/498] Linking CXX executable src/bitcoin-wallet
[381/498] Building CXX object src/CMakeFiles/server.dir/net_processing.cpp.o
FAILED: src/CMakeFiles/server.dir/net_processing.cpp.o 
/usr/bin/ccache /usr/bin/clang++  -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-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/net_processing.cpp.o -MF src/CMakeFiles/server.dir/net_processing.cpp.o.d -o src/CMakeFiles/server.dir/net_processing.cpp.o -c ../../src/net_processing.cpp
../../src/net_processing.cpp:1435:25: error: loop variable 'ptx' of type 'const std::shared_ptr<const CTransaction>' creates a copy from type 'const std::shared_ptr<const CTransaction>' [-Werror,-Wrange-loop-construct]
        for (const auto ptx : pblock->vtx) {
                        ^
../../src/net_processing.cpp:1435:14: note: use reference type 'const std::shared_ptr<const CTransaction> &' to prevent copying
        for (const auto ptx : pblock->vtx) {
             ^~~~~~~~~~~~~~~~
                        &
1 error generated.
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

address build-clang failure:

[11:14:03]
[Step 1/1] FAILED: src/CMakeFiles/server.dir/net_processing.cpp.o
[11:14:03]
[Step 1/1] /usr/bin/ccache /usr/bin/clang++  -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-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++17 -MD -MT src/CMakeFiles/server.dir/net_processing.cpp.o -MF src/CMakeFiles/server.dir/net_processing.cpp.o.d -o src/CMakeFiles/server.dir/net_processing.cpp.o -c ../../src/net_processing.cpp
[11:14:03]
[Step 1/1] ../../src/net_processing.cpp:1435:25: error: loop variable 'ptx' of type 'const std::shared_ptr<const CTransaction>' creates a copy from type 'const std::shared_ptr<const CTransaction>' [-Werror,-Wrange-loop-construct]
[11:14:03]
[Step 1/1]         for (const auto ptx : pblock->vtx) {
[11:14:03]
[Step 1/1]                         ^
[11:14:03]
[Step 1/1] ../../src/net_processing.cpp:1435:14: note: use reference type 'const std::shared_ptr<const CTransaction> &' to prevent copying
[11:14:03]
[Step 1/1]         for (const auto ptx : pblock->vtx) {
[11:14:03]
[Step 1/1]              ^~~~~~~~~~~~~~~~
[11:14:03]
[Step 1/1]

Harbormaster completed remote builds in B14443: Diff 26425.Dec 18 2020, 11:42

majcosta requested changes to this revision.Dec 18 2020, 18:56

majcosta added a subscriber: majcosta.

majcosta added inline comments.

src/net_processing.cpp
1436 ↗	(On Diff #26425)	I'm pretty sure we usually translate GetHash() to GetId() on our codebase where transactions are concerned

This revision now requires changes to proceed.Dec 18 2020, 18:56

PiRK edited the summary of this revision. (Show Details)Dec 19 2020, 11:48

Use TxId instead of TxHash

@@ -1433,7 +1433,7 @@ void PeerLogicValidation::BlockConnected(
     {
         LOCK(g_cs_recent_confirmed_transactions);
         for (const CTransactionRef &ptx : pblock->vtx) {
-            g_recent_confirmed_transactions->insert(ptx->GetHash());
+            g_recent_confirmed_transactions->insert(ptx->GetId());
         }
     }
 }
@@ -1619,7 +1619,7 @@ static bool AlreadyHave(const CInv &inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
 
             {
                 LOCK(g_cs_recent_confirmed_transactions);
-                if (g_recent_confirmed_transactions->contains(inv.hash)) {
+                if (g_recent_confirmed_transactions->contains(TxId{inv.hash})) {
                     return true;
                 }
             }

Harbormaster completed remote builds in B14458: Diff 26450.Dec 19 2020, 12:06

deadalnix requested changes to this revision.Dec 19 2020, 13:16

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/net_processing.cpp
209 ↗	(On Diff #26450)	Double star
1622 ↗	(On Diff #26450)	If doesn't looks necessary to create a TxId object here. If it is then it redundant to do it twice anyways.

This revision now requires changes to proceed.Dec 19 2020, 13:16

PiRK added inline comments.Dec 21 2020, 09:08

src/net_processing.cpp
1622 ↗	(On Diff #26450)	I will change the code so that the `TxId` object is created only once, instead of 3 times. If we want to use the hash instead of txid, we also need to change the type of `mapOrphanTransactions`. That should be done in a separate diff.

create the TxId object earlier so that it can be used everywhere, instead of recreating the object 3 times

Harbormaster completed remote builds in B14460: Diff 26455.Dec 21 2020, 09:26

majcosta requested changes to this revision.Dec 21 2020, 14:19

majcosta added inline comments.

src/net_processing.cpp
1628 ↗	(On Diff #26455)	might as well

This revision now requires changes to proceed.Dec 21 2020, 14:19

PiRK added inline comments.Dec 21 2020, 14:27

src/net_processing.cpp
1628 ↗	(On Diff #26455)	You will have to teach me how you format a beautifull diff in a reply.

use txid also when checking if transaction is in recentRejects

This is consistent with recentRejects->insert(tx.GetId()) (line 3289)

majcosta accepted this revision.Dec 21 2020, 14:36

Harbormaster completed remote builds in B14482: Diff 26477.Dec 21 2020, 14:48

deadalnix added inline comments.Dec 23 2020, 01:19

src/net_processing.cpp
1622 ↗	(On Diff #26450)	mapOrphanTransactions must not change type as this is a map containing txids. bloom filter do not accept txids, but uint256, they are agnostic to what's in the map, and therefore creating a txid for these is completely redundant work.

deadalnix accepted this revision.Dec 23 2020, 01:20

This revision is now accepted and ready to land.Dec 23 2020, 01:20

Closed by commit rABC1d95857cbced: Use rolling bloom filter of recent block tx's for AlreadyHave() check (authored by Suhas Daftuar <sdaftuar@gmail.com>, committed by PiRK). · Explain WhyDec 23 2020, 09:37

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC1d95857cbced: Use rolling bloom filter of recent block tx's for AlreadyHave() check.

Use rolling bloom filter of recent block tx's for AlreadyHave() check
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26537

src/net_processing.h

src/net_processing.cpp

Use rolling bloom filter of recent block tx's for AlreadyHave() checkClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26537

src/net_processing.h

src/net_processing.cpp

Use rolling bloom filter of recent block tx's for AlreadyHave() check
ClosedPublic
Actions

Revision Contents
Changeset List