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.
55c4795c57 [net processing] Use TxRelay::m_relay_txs over CNode::m_relays_txs (dergoegge)
Pull request description:
`CNode::m_relays_txs` is meant to only be used for the eviction logic in `net`. `TxRelay::m_relay_txs` will hold the same value and is meant to be used on the application layer to determine if we will/should relay transactions to a peer.
(Shameless plug: we should really better specify the interface for updating eviction data to avoid refactors like this in the future -> #25572)
ACKs for top commit:
MarcoFalke:
lgtm ACK 55c4795c57
Tree-SHA512: 59cfd23e32568fd96cda5570790e518242a6c76d4edf5b7d1a2a7f9724d590d2a38395504e05be0af4e98dd5c0056fc0be6568eab2818934692483a186e5181d
Taking cs_main is no longer necessary since we moved
`m_recently_announced_invs` to `Peer` and `mapRelay` is actually only
accessed from the message processing thread.
3566aa7d49 [net] Remove CNode friends (dergoegge)
3eac5e7cd1 [net] Add CNode helper for send byte accounting (dergoegge)
60441a3432 scripted-diff: [net] Rename CNode process queue members (dergoegge)
6693c499f7 [net] Make cs_vProcessMsg a non-recursive mutex (dergoegge)
23d9352654 [net] Make CNode msg process queue members private (dergoegge)
897e342d6e [net] Encapsulate CNode message polling (dergoegge)
cc5cdf8776 [net] Deduplicate marking received message for processing (dergoegge)
ad44aa5c64 [net] Add connection type getter to CNode (dergoegge)
Pull request description:
We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and its mutex is turned into a non-recursive mutex.
ACKs for top commit:
jnewbery:
utACK 3566aa7d49
vasild:
ACK 3566aa7d49
theStack:
re-ACK 3566aa7d49
brunoerg:
re-ACK 3566aa7d49
Tree-SHA512: 26b87da5054e32401b693b2904e9c5f40e35a53937c0b6cf44b8597034ad07bacf27d87cdffc54d3e7ccfebde4231ef30a38d326f88cc18133bbb34688ead567
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>
c9d548c91f net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e9 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20 net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59a scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91f
achow101:
ACK c9d548c91f
jonatack:
re-ACK c9d548c91f only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91f
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
faf7b4f1fc Add BlockManager::IsPruneMode() (MarcoFalke)
fae71fe27e Add BlockManager::GetPruneTarget() (MarcoFalke)
fa0f0436d8 Add BlockManager::LoadingBlocks() (MarcoFalke)
Pull request description:
Requested in https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1061323795, but adding getters seems unrelated from removing globals, so I split it out for now.
ACKs for top commit:
dergoegge:
Code review ACK faf7b4f1fc
brunoerg:
crACK faf7b4f1fc
Tree-SHA512: 204d0e9a0e8b78175482f89b4ce620fba0e65d8e49ad845d187af44d3843f4c733a01bac1ffe5a5319f524d8346123693a456778b69d6c75268c447eb8839642
39b93649c4 test: add functional test for IBD stalling logic (Martin Zumsande)
0565951f34 p2p: Make block stalling timeout adaptive (Martin Zumsande)
Pull request description:
During IBD, there is the following stalling mechanism if we can't proceed with assigning blocks from a 1024 lookahead window because all of these blocks are either already downloaded or in-flight: We'll mark the peer from which we expect the current block that would allow us to advance our tip (and thereby move the 1024 window ahead) as a possible staller. We then give this peer 2 more seconds to deliver a block (`BLOCK_STALLING_TIMEOUT`) and if it doesn't, disconnect it and assign the critical block we need to another peer.
Now the problem is that this second peer is immediately marked as a potential staller using the same mechanism and given 2 seconds as well - if our own connection is so slow that it simply takes us more than 2 seconds to download this block, that peer will also be disconnected (and so on...), leading to repeated disconnections and no progress in IBD. This has been described in #9213, and I have observed this when doing IBD on slower connections or with Tor - sometimes there would be several minutes without progress, where all we did was disconnect peers and find new ones.
The `2s` stalling timeout was introduced in #4468, when blocks weren't full and before Segwit increased the maximum possible physical size of blocks - so I think it made a lot of sense back then.
But it would be good to revisit this timeout now.
This PR makes the timout adaptive (idea by sipa):
If we disconnect a peer for stalling, we now double the timeout for the next peer (up to a maximum of 64s). If we connect a block, we half it again up to the old value of 2 seconds. That way, peers that are comparatively slower will still get disconnected, but long phases of disconnecting all peers shouldn't happen anymore.
Fixes#9213
ACKs for top commit:
achow101:
ACK 39b93649c4
RandyMcMillan:
Strong Concept ACK 39b93649c4
vasild:
ACK 39b93649c4
naumenkogs:
ACK 39b93649c4
Tree-SHA512: 85bc57093b2fb1d28d7409ed8df5a91543909405907bc129de7c6285d0810dd79bc05219e4d5aefcb55c85512b0ad5bed43a4114a17e46c35b9a3f9a983d5754
fa035fe2d6 Remove unused CDataStream::SetType (MarcoFalke)
fa29e73cda Use DataStream where possible (MarcoFalke)
fa9becfe1c streams: Add DataStream without ser-type and ser-version (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `DataStream`. `CDataStream` remains in places where it is not yet possible.
ACKs for top commit:
stickies-v:
re-ACK [fa035fe](fa035fe2d6)
aureleoules:
diff re-ACK fa035fe2d6fa0e6640ba..fa035fe2d6
Tree-SHA512: cb5e53d0df7c94319ffadc6ea1d887fc38516decaf43f0673396d79cc62d450a1a61173654a91b8c2b52d2cecea53fe4a500b8f6466596f35731471163fb051c
c58c249a5b net_processing: indicate more work to do when orphans are ready to reconsider (Anthony Towns)
ecb0a3e425 net_processing: Don't process tx after processing orphans (Anthony Towns)
c583775706 net_processing: only process orphans before messages (Anthony Towns)
be2304676b txorphange: Drop redundant originator arg from GetTxToReconsider (Anthony Towns)
a4fe09973a txorphanage: index workset by originating peer (Anthony Towns)
Pull request description:
We currently process orphans by assigning them to the peer that provided a missing parent; instead assign them to the peer that provided the orphan in the first place. This prevents a peer from being able to marginally delay another peer's transactions and also simplifies the internal API slightly. Because we're now associating orphan processing with the peer that provided the orphan originally, we no longer process orphans immediately after receiving the parent, but defer until a future call to `ProcessMessage`.
Based on #26295
ACKs for top commit:
naumenkogs:
utACK c58c249a5b
glozow:
ACK c58c249a5b
mzumsande:
Code Review ACK c58c249a5b
Tree-SHA512: 3186c346f21e60440266a2a80a9d23d7b96071414e14b2b3bfe50457c04c18b1eab109c3d8c2a7726a6b10a2eda1f0512510a52c102da112820a26f5d96f12de
