I commented out a few new lines at the bottom of test/functional/p2p_ibd_txrelay.py
It tests that nodes process certain transactions, after IBD. These new assertions
from upstream are failing, but it may be because our IBD / tx sharing logic is different.
We should revisit this.
We comment out a newly-added assert from the Bitcoin
side, because it's unclear how to replicate it
correctly in Elements.
Co-authored-by: Glenn Willen <gwillen@nerdnet.org>
When receiving unwanted headers from a peer, first check whether we have
recent "best known block" state for that peer. If not, accept one batch
of headers to refresh it. Otherwise, we can become stuck in a state where
we need blocks, but we don't know which peers have those blocks available.
This especially happens during startup, when we don't have this information
for any peers until we start processing header messages.
eaf6be0114 [net processing] Do not request transaction relay from feeler connections (John Newbery)
0220b834b1 [test] Add testing for outbound feeler connections (John Newbery)
Pull request description:
Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a `version` message from the peer, and then close the connection.
Currently, we set `fRelay` to `1` in the `version` message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the `version` message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set `fRelay` to `0` indicating that we do not wish to receive transaction announcements from the peer.
This PR also extends the `addconnection` RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets `fRelay` to `0` in the `version` message to feeler connections.
ACKs for top commit:
naumenkogs:
ACK eaf6be0114
MarcoFalke:
review ACK eaf6be0114🏃
Tree-SHA512: 1c56837dbd0a396fe404a5e39f7459864d15f666664d6b35ad109628b13158e077e417e586bf48946a23bd5cbe63716cb4bf22cdf8781b74dfce6047b87b465a
fadc0c80ae p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke)
Pull request description:
Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836Fixes#20654
ACKs for top commit:
laanwj:
Code review ACK fadc0c80ae
naumenkogs:
ACK fadc0c80ae
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
dce8c4c381 rpc: getblockfrompeer (Sjors Provoost)
b884ababc2 rpc: move Ensure* helpers to server_util.h (Sjors Provoost)
Pull request description:
This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (`headers-only` in `getchaintips`).
Usage:
```
bitcoin-cli getblockfrompeer HASH peer_n
```
Closes#20155
Limitations:
* you have to specify which peer to fetch the block from
* the node must already have the header
ACKs for top commit:
jnewbery:
ACK dce8c4c381
fjahr:
re-ACK dce8c4c381
Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8
0c85dc30e6 p2p: Don't use timestamps from inbound peers (Martin Zumsande)
Pull request description:
`GetAdjustedTime()` (used e.g. in validation and addrman) returns a time with an offset that is influenced by timestamps that our peers have sent us in their version message.
Currently, timestamps from all peers are used for this.
However, I think that it would make sense to ignore the timedata samples from inbound peers, making it much harder for others to influence the Adjusted Time in a targeted way.
With the extra feeler connections (every 2 minutes on average) and extra block-relay-only connections (every 5 minutes on average) there are also now plenty of opportunities to gather a meaningful number of timedata samples from outbound peers.
There are some measures in place to prevent abuse: the `-maxtimeadjustment` parameter with a default of 70 minutes, warnings in cases of large deviations, only using the first 200 samples ([explanation](383d350bd5/src/timedata.cpp (L57-L72))), but I think that only using samples from outbound connections in the first place would be an additional safety measure that would make sense.
See also issue #4521 for further context and links: There have been several discussions in the past about replacing or abolishing the existing timedata system.
ACKs for top commit:
jnewbery:
Concept and code review ACK 0c85dc30e6
naumenkogs:
ACK 0c85dc30e6
vasild:
ACK 0c85dc30e6
Tree-SHA512: 2d6375305bcae034d68b58b7a07777b40ac430dfed554c88e681a048c527536691e1b7d08c0ef995247d356f8e81aa0a4b983bf2674faf6a416264e5f1af0a96
11daf6ceb1 More Span simplifications (Pieter Wuille)
568dd2f839 Replace MakeSpan helper with Span deduction guide (Pieter Wuille)
Pull request description:
C++17 supports [user-defined deduction guides](https://en.cppreference.com/w/cpp/language/class_template_argument_deduction), allowing class constructors to be invoked without specifying class template arguments. Instead, the code can contain rules to infer the template arguments from the constructor argument types.
This alleviates the need for the `MakeSpan` helper. Convert the existing MakeSpan rules into deduction rules for `Span` itself, and replace all invocations of `MakeSpan` with just `Span` ones.
ACKs for top commit:
MarcoFalke:
re-ACK 11daf6ceb1 Only change is removing a hunk in the tests 🌕
Tree-SHA512: 10f3e82e4338f39d9b7b407cd11aac7ebe1e9191b58e3d7f4e5e338a4636c0e126b4a1d912127c7446f57ba356c8d6544482e47f97901efea6a54fffbfd7895f
6aed8b7e9b [test] tx processing before and after ibd (glozow)
b9e105b664 [net_processing] ignore all transactions during ibd (glozow)
Pull request description:
This is basically a mini, IBD-only version of #21224
Incoming transactions aren't really relevant until we're caught up. That's why we send a giant feefilter and don't send tx getdatas, but we also shouldn't process them if peers send them anyway. Simply ignore them.
ACKs for top commit:
jnewbery:
reACK 6aed8b7e9b
laanwj:
Code review ACK 6aed8b7e9b
Tree-SHA512: 8e1616bf355f9d0b180bdbc5461f24c757dc5d7bc7bf651470f3b0bffcca5d5e68287106255b5cede2d96b42bce448a0f8c0649de35a530c5e079f7c89c70a35
This fixes an issue which causes Elements to download the blockchain headers
multiple times during initial block download.
In particular: each time we receive an INV P2P message with a new block
(about once a minute), we start downloading the headers, again, in parallel
with any existing download(s) in progress.
With this change, after we receive each batch of headers, we check whether
any of the headers in it were new to us. If not (they were all duplicates),
we stop there, and do not ask the peer for another batch. This reduces the
maximum amount of duplication to about 2x, which is not ideal, but a HUGE
improvement.
