Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace global trickle node with random delays #7125

Merged
merged 1 commit into from Dec 14, 2015

Conversation

sipa
Copy link
Member

@sipa sipa commented Nov 28, 2015

This is based upon and partially replaces #5989, and rebased on top of #7100.

We used to have a trickle node, a node which was chosen in each iteration of the send loop that was privileged and allowed to send out queued up non-time critical messages. Since the removal of the fixed sleeps in the network code, this resulted in fast and attackable (see #7123) treatment of such broadcasts.

This pull request changes each of these trickle-dependant mechanisms with a per-node and per-feature random delay timer, improving privacy and performance by making queueing useful again.

The commits are mostly squashed/rebased versions taken from #5989 by @pstratem, except:

@sipa
Copy link
Member Author

sipa commented Nov 29, 2015

Rebased on top of #7125, and make blocks not use the vInventoryToSend system anymore (making it safer to make it filter like #7100).

@sipa
Copy link
Member Author

sipa commented Nov 30, 2015

Rebased on top of #7133.

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 5, 2015

Could use some rebasing love, as the bloomy stuff was merged.

@sipa
Copy link
Member Author

sipa commented Dec 5, 2015

Rebased.

@laanwj laanwj added this to the 0.12.0 milestone Dec 9, 2015
@laanwj
Copy link
Member

laanwj commented Dec 9, 2015

Some nodes connect to to as many peers as possible and flood them with pings, apparently to trigger the issue where pings interfere with trickle delays. So this is being actively exploited.

Added 0.12 milestone.

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 10, 2015

I am currently testing this on top of 0.12.

@morcos
Copy link
Member

morcos commented Dec 10, 2015

utACK

{

if (pto->nNextLocalAddrSend < nNow) {
AdvertizeLocal(pto);
Copy link
Member

@sdaftuar sdaftuar Dec 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be guarding this with a !IsInitialBlockDownload() as before?

@sipa
Copy link
Member Author

sipa commented Dec 11, 2015

Addressed @sdaftuar's comment, rebased, and switched to Poisson-distributed sends, so nobody can ever make a guess about when the next send will happen.

We used to have a trickle node, a node which was chosen in each iteration of
the send loop that was privileged and allowed to send out queued up non-time
critical messages. Since the removal of the fixed sleeps in the network code,
this resulted in fast and attackable treatment of such broadcasts.

This pull request changes the 3 remaining trickle use cases by random delays:
* Local address broadcast (while also removing the the wiping of the seen filter)
* Address relay
* Inv relay (for transactions; blocks are always relayed immediately)

The code is based on older commits by Patrick Strateman.
@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 14, 2015

ACK

@laanwj laanwj merged commit 5400ef6 into bitcoin:master Dec 14, 2015
laanwj added a commit that referenced this pull request Dec 14, 2015
5400ef6 Replace trickle nodes with per-node/message Poisson delays (Pieter Wuille)
sipa added a commit that referenced this pull request Dec 14, 2015
We used to have a trickle node, a node which was chosen in each iteration of
the send loop that was privileged and allowed to send out queued up non-time
critical messages. Since the removal of the fixed sleeps in the network code,
this resulted in fast and attackable treatment of such broadcasts.

This pull request changes the 3 remaining trickle use cases by random delays:
* Local address broadcast (while also removing the the wiping of the seen filter)
* Address relay
* Inv relay (for transactions; blocks are always relayed immediately)

The code is based on older commits by Patrick Strateman.

Github-Pull: #7125
Rebased-From: 5400ef6
@laanwj
Copy link
Member

laanwj commented Feb 4, 2016

This is cherry-picked to 0.12 as 10b88be

@dgenr8
Copy link
Contributor

dgenr8 commented Mar 17, 2018

Where was the discussion of the choice of 5 seconds as the mean delay between inventory broadcasts? This was 7-10x the median transaction propagation time of the entire network at that time.

random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jul 1, 2020
6f41b3e [QA] Missing mempool sync in pos_coldStaking and zc_publicspends tests (random-zebra)
efaf727 net: correctly initialize nMinPingUsecTime (Wladimir J. van der Laan)
61c8ffe Do not add random inbound peers to addrman. (Gregory Maxwell)
e6a1726 Added feeler connections increasing good addrs in the tried table. (Ethan Heilman)
070b6fb Actually only use filterInventoryKnown with MSG_TX inventory messages. (Gregory Maxwell)
9ac6b28 Only use filterInventoryKnown with MSG_TX inventory messages. (Patick Strateman)
01273db Rename setInventoryKnown filterInventoryKnown (Patick Strateman)
4f11eb2 Remove mruset as it is no longer used. (Gregory Maxwell)
2e3b05c Replace setInventoryKnown with a rolling bloom filter. (Gregory Maxwell)
409aa83 Replace trickle nodes with per-node/message Poisson delays (Pieter Wuille)
93e8c46 Move recentRejects initialization to top of InitBlockIndex (Wladimir J. van der Laan)
9a59420 Keep track of recently rejected transactions (Peter Todd)
34ee777 Only use randomly created nonces in CRollingBloomFilter. (Pieter Wuille)
338d346 Make CRollingBloomFilter set nTweak for you (Peter Todd)
dcd15bc Reuse vector hashing code for uint256 (Pieter Wuille)
3230143 Add uint256 support to CRollingBloomFilter (Peter Todd)
128d644 Better mruset unit test (Pieter Wuille)
89740ed Use ring buffer of set iterators instead of deque of copies in mruset (Pieter Wuille)
14c88ee Replace mruset setAddrKnown with CRollingBloomFilter addrKnown (Gavin Andresen)
e0bebbd Rolling bloom filter class (Gavin Andresen)
7c03bd5 Add correct bool combiner for net signals (Pieter Wuille)
5cb5fd6 Stop exporting ConnectNode (Fuzzbawls)
819295d Stop using ConnectNode in layer 2 code (Fuzzbawls)
851b6b4 net: No need to export DumpBanlist (Cory Fields)
4486d4e net: make Ban/Unban/ClearBan functionality consistent (Cory Fields)
a5e278d net: Drop CNodeRef for AttemptToEvictConnection (Cory Fields)
9fd357d net: use the exposed GetNodeSignals() rather than g_signals directly (Cory Fields)
7962bcc net: remove unused set (Cory Fields)
fabf358 Use network group instead of CNetAddr in final pass to select node to disconnect (Patrick Strateman)
7f030fe Fix comment (Patrick Strateman)
18af800 Acquire cs_vNodes before changing refrence counts (Patrick Strateman)
7aa827f CNodeRef copy constructor and assignment operator (Patrick Strateman)
b3f95e7 Return false early if vEvictionCandidates is empty (Patrick Strateman)
85886c9 Better support for nodes with non-standard nMaxConnections (Patrick Strateman)
9c9e55b RAII wrapper for CNode* (Patrick Strateman)
e92780d Add comments to AttemptToEvictConnection (Patrick Strateman)
0ca7ce3 Prefer to disconnect peers in favor of whitelisted peers (Patrick Strateman)
a1c4aaf AttemptToEvictConnection (Patrick Strateman)
aa7ce9b Record nMinPingUsecTime (Patrick Strateman)
fd7bab0 Refactor: Move failure conditions to the top of AcceptConnection (Patrick Strateman)
fcb732b Refactor: Bail early in AcceptConnection (Patrick Strateman)
411766d Refactor: AcceptConnection (Patrick Strateman)

Pull request description:

  This is a culmination of several upstream PRs touching the P2P/Networking code, and resulting in a state just prior to the P2P/Network encapsulation, which will be it's own PR.

  Backported upstream PRs included here:
  - bitcoin#5859
  - bitcoin#6064
  - bitcoin#6374
  - bitcoin#6498
  - bitcoin#6636
  - bitcoin#7133
  - bitcoin#7125
  - bitcoin#7906
  - bitcoin#8282
  - bitcoin#8594

ACKs for top commit:
  random-zebra:
    Great job. ACK 6f41b3e and merging...
  furszy:
    Have been running it the past days and all is looking good, ACK 6f41b3e .

Tree-SHA512: 1cc4b1271516f000a06141b5b069f3ee00f3eb77056e40d2c021c484f749d9d8db2b76ce490f63572372705b646fad342666f6f90ca5fc69abcacf7b207d058f
laanwj added a commit that referenced this pull request Dec 7, 2020
65273fa Clear m_addr_known before our periodic self-advertisement (Suhas Daftuar)

Pull request description:

  We use a rolling bloom filter to track which addresses we've previously sent a peer, but after #7125 we no longer clear it every day before our own announcement.  This looks to me like an oversight which has the effect of reducing the frequency with which we actually self-announce our own address, so this reintroduces resetting that filter.

ACKs for top commit:
  naumenkogs:
    ACK 65273fa
  laanwj:
    Code review ACK 65273fa
  sipa:
    utACK 65273fa

Tree-SHA512: 602c155fb6d2249b054fcb6f1c0dd17143605ceb87132286bbd90babf26d258ff6c41f9925482c17e2be41805d33f9b83926cb447f394969ffecd4bccfa0a64f
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2020
65273fa Clear m_addr_known before our periodic self-advertisement (Suhas Daftuar)

Pull request description:

  We use a rolling bloom filter to track which addresses we've previously sent a peer, but after bitcoin#7125 we no longer clear it every day before our own announcement.  This looks to me like an oversight which has the effect of reducing the frequency with which we actually self-announce our own address, so this reintroduces resetting that filter.

ACKs for top commit:
  naumenkogs:
    ACK 65273fa
  laanwj:
    Code review ACK 65273fa
  sipa:
    utACK 65273fa

Tree-SHA512: 602c155fb6d2249b054fcb6f1c0dd17143605ceb87132286bbd90babf26d258ff6c41f9925482c17e2be41805d33f9b83926cb447f394969ffecd4bccfa0a64f
zkbot added a commit to zcash/zcash that referenced this pull request Aug 4, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
zkbot added a commit to zcash/zcash that referenced this pull request Aug 5, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
zkbot added a commit to zcash/zcash that referenced this pull request Aug 5, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
zkbot added a commit to zcash/zcash that referenced this pull request Aug 9, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
zkbot added a commit to zcash/zcash that referenced this pull request Aug 9, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
zkbot added a commit to zcash/zcash that referenced this pull request Aug 9, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
zkbot added a commit to zcash/zcash that referenced this pull request Aug 10, 2021
ZIP 239 preparations 1

This is the first of several backports to prepare for ZIP 239. The primary
change is altering `mapRelay` to store `CTransaction`s, which we need
because ZIP 239 requires changing `Inv` messages based on transaction
versions. The other changes are mainly for conflict removal but are also
independently useful.

Backports the following upstream PRs:
- bitcoin/bitcoin#6889
- bitcoin/bitcoin#7125
- bitcoin/bitcoin#7862
- bitcoin/bitcoin#7877
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants