Skip to content

Commit c2c4dba

Browse files
committed
Merge #19988: Overhaul transaction request logic
fd9a006 Report and verify expirations (Pieter Wuille) 86f50ed Delete limitedmap as it is unused now (Pieter Wuille) cc16fff Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille) 173a1d2 Expedite removal of tx requests that are no longer needed (Pieter Wuille) de11b0a Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille) 242d164 Change transaction request logic to use txrequest (Pieter Wuille) 5b03121 Add txrequest fuzz tests (Pieter Wuille) 3c7fe0e Add txrequest unit tests (Pieter Wuille) da3b8fd Add txrequest module (Pieter Wuille) Pull request description: This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests). The major changes are: * Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first. * No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available). * The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions). This replaces #19184, rebased on #18044 and with many small changes. ACKs for top commit: ariard: Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic. MarcoFalke: Approach ACK fd9a006 🏹 naumenkogs: Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events. jnewbery: utACK fd9a006 jonatack: WIP light ACK fd9a006 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers. ryanofsky: Light code review ACK fd9a006, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on: Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7
2 parents 99a1d57 + fd9a006 commit c2c4dba

18 files changed

+2297
-447
lines changed

doc/release-notes-19988.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
P2P changes
2+
-----------
3+
4+
The size of the set of transactions that peers have announced and we consider
5+
for requests has been reduced from 100000 to 5000 (per peer), and further
6+
announcements will be ignored when that limit is reached. If you need to
7+
dump (very) large batches of transactions, exceptions can be made for trusted
8+
peers using the "relay" network permission. For localhost for example it can
9+
be enabled using the command line option `[email protected]`.

src/Makefile.am

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,6 @@ BITCOIN_CORE_H = \
151151
interfaces/wallet.h \
152152
key.h \
153153
key_io.h \
154-
limitedmap.h \
155154
logging.h \
156155
logging/timer.h \
157156
memusage.h \
@@ -215,6 +214,7 @@ BITCOIN_CORE_H = \
215214
timedata.h \
216215
torcontrol.h \
217216
txdb.h \
217+
txrequest.h \
218218
txmempool.h \
219219
undo.h \
220220
util/asmap.h \
@@ -327,6 +327,7 @@ libbitcoin_server_a_SOURCES = \
327327
timedata.cpp \
328328
torcontrol.cpp \
329329
txdb.cpp \
330+
txrequest.cpp \
330331
txmempool.cpp \
331332
validation.cpp \
332333
validationinterface.cpp \

src/Makefile.test.include

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ FUZZ_TARGETS = \
151151
test/fuzz/tx_in_deserialize \
152152
test/fuzz/tx_out \
153153
test/fuzz/txoutcompressor_deserialize \
154+
test/fuzz/txrequest \
154155
test/fuzz/txundo_deserialize \
155156
test/fuzz/uint160_deserialize \
156157
test/fuzz/uint256_deserialize
@@ -235,7 +236,6 @@ BITCOIN_TESTS =\
235236
test/interfaces_tests.cpp \
236237
test/key_io_tests.cpp \
237238
test/key_tests.cpp \
238-
test/limitedmap_tests.cpp \
239239
test/logging_tests.cpp \
240240
test/dbwrapper_tests.cpp \
241241
test/validation_tests.cpp \
@@ -275,6 +275,7 @@ BITCOIN_TESTS =\
275275
test/torcontrol_tests.cpp \
276276
test/transaction_tests.cpp \
277277
test/txindex_tests.cpp \
278+
test/txrequest_tests.cpp \
278279
test/txvalidation_tests.cpp \
279280
test/txvalidationcache_tests.cpp \
280281
test/uint256_tests.cpp \
@@ -1214,6 +1215,12 @@ test_fuzz_txoutcompressor_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)
12141215
test_fuzz_txoutcompressor_deserialize_LDFLAGS = $(FUZZ_SUITE_LDFLAGS_COMMON)
12151216
test_fuzz_txoutcompressor_deserialize_SOURCES = test/fuzz/deserialize.cpp
12161217

1218+
test_fuzz_txrequest_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
1219+
test_fuzz_txrequest_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
1220+
test_fuzz_txrequest_LDADD = $(FUZZ_SUITE_LD_COMMON)
1221+
test_fuzz_txrequest_LDFLAGS = $(FUZZ_SUITE_LDFLAGS_COMMON)
1222+
test_fuzz_txrequest_SOURCES = test/fuzz/txrequest.cpp
1223+
12171224
test_fuzz_txundo_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DTXUNDO_DESERIALIZE=1
12181225
test_fuzz_txundo_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
12191226
test_fuzz_txundo_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)

src/limitedmap.h

Lines changed: 0 additions & 100 deletions
This file was deleted.

src/net.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
#include <compat.h>
1515
#include <crypto/siphash.h>
1616
#include <hash.h>
17-
#include <limitedmap.h>
1817
#include <net_permissions.h>
1918
#include <netaddress.h>
2019
#include <optional.h>

src/net_permissions.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
1212
"bloomfilter (allow requesting BIP37 filtered blocks and transactions)",
1313
"noban (do not ban for misbehavior; implies download)",
1414
"forcerelay (relay transactions that are already in the mempool; implies relay)",
15-
"relay (relay even in -blocksonly mode)",
15+
"relay (relay even in -blocksonly mode, and unlimited transaction announcements)",
1616
"mempool (allow requesting BIP35 mempool contents)",
1717
"download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)",
1818
"addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)"

src/net_permissions.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ enum NetPermissionFlags {
1919
// Can query bloomfilter even if -peerbloomfilters is false
2020
PF_BLOOMFILTER = (1U << 1),
2121
// Relay and accept transactions from this peer, even if -blocksonly is true
22+
// This peer is also not subject to limits on how many transaction INVs are tracked
2223
PF_RELAY = (1U << 3),
2324
// Always relay transactions from this peer, even if already in mempool
2425
// Keep parameter interaction: forcerelay implies relay

0 commit comments

Comments
 (0)