| Age | Commit message (Collapse) | Author |
|---|
|
|
|
immediate discouragement
6eecba475efd025eb011400af58621ad5823994e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da33f238ed2186799da4e109d4edd3a1 net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c311977bba3585648e32e3bd5754028aa292 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478c200b18ee621a6ffa0362f4e991959 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d5c081dc433dae7e7941074a3a8b5a7 net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1931c4d7c36abbb000b7de306d83a4c net_processing: do not treat non-connecting headers as response (Pieter Wuille)
Pull request description:
So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.
This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.
The specific types of misbehavior that are changed to 100 are:
* Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
* Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
* Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
* Sending us more than 50000 invs in a single `inv` message [used to be score 20]
* Sending us more than 2000 headers in a single `headers` message [used to be score 20]
The specific types of misbehavior that are changed to 0 are:
* Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
* Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]
I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.
In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.
ACKs for top commit:
sr-gi:
ACK [6eecba4](https://github.com/bitcoin/bitcoin/pull/29575/commits/6eecba475efd025eb011400af58621ad5823994e)
achow101:
ACK 6eecba475efd025eb011400af58621ad5823994e
mzumsande:
Code Review ACK 6eecba475efd025eb011400af58621ad5823994e
glozow:
light code review / concept ACK 6eecba475efd025eb011400af58621ad5823994e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
|
|
|
|
This is now all unused.
|
|
|
|
For querying statistics/info from PeerManager. The median outbound time
offset is the only initial field.
|
|
|
|
|
|
No behavior change. Just an intermediate refactoring.
By relocating the peer desirable services flags into the peer
manager, we allow the connections acceptance process to handle
post-IBD potential stalling scenarios.
In the follow-up commit(s), the desirable service flags will be
dynamically adjusted to detect post-IBD stalling scenarios (such
as a +48-hour inactive node that must prefer full node connections
instead of limited peer connections because they cannot provide
historical blocks). Additionally, this encapsulation enable us
to customize the connections decision-making process based on
new user's configurations in the future.
|
|
And implement 'ApproximateBestBlockDepth()' to estimate
the distance, in blocks, between the best-known block
and the network chain tip. Utilizing the best-block time
and the chainparams blocks spacing to approximate it.
|
|
|
|
|
|
Also changes max_extra_txs into a uint32_t to avoid platform-specific
behaviour
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
A single outbound slot is required, so if the first two slots
are taken by inbound in-flights, the node will reject additional
unless they are coming from outbound.
This means in the case where a fast sybil peer is attempting to
stall out a node, a single high bandwidth outbound peer can
mitigate the attack.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
There are many cases where we assume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.
|
|
|
|
Use Peer::m_their_services instead
|
|
`Misbehaving` has several coding related issues (ignoring the conceptual
issues here for now):
* It is public, but it is not supposed to be called from outside of
net_processing. Fix that by making it private and creating a public
`UnitTestMisbehaving` method for unit testing only.
* It doesn't do anything if a `nullptr` is passed. It would be less
confusing to just skip the call instead. Fix that by passing `Peer&`
to `Misbehaving()`.
* It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be
propagated. This is harmless, but verbose. Fix it by removing the no
longer needed call to `GetPeerRef` and the no longer needed lock
annotations.
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
ren cs_filter m_bloom_filter_mutex
ren fRelayTxes m_relay_txs
ren pfilter m_bloom_filter
ren cs_tx_inventory m_tx_inventory_mutex
ren filterInventoryKnown m_tx_inventory_known_filter
ren setInventoryTxToSend m_tx_inventory_to_send
ren fSendMempool m_send_mempool
ren nNextInvSend m_next_inv_send_time
ren minFeeFilter m_fee_filter_received
ren lastSentFeeFilter m_fee_filter_sent
-END VERIFY SCRIPT-
|
|
|
|
923312fbf6a89efde1739da0b7209694d4f892ba rpc: use peer_id, block_hash for FetchBlock (Sjors Provoost)
34d5399211eeb61e7e7961c301fb2ddea8aa3f6a rpc: more detailed errors for getblockfrompeer (Sjors Provoost)
60243cac7286e4c4bdda7094bef4cf6d1564b583 rpc: turn already downloaded into error in getblockfrompeer (Sjors Provoost)
809d66bb65aa78048e27c2a878d6f7becaecfe11 rpc: clarify getblockfrompeer behavior when called multiple times (Sjors Provoost)
0e3d7c5ee16d5a4c061ab9a57285bceb7899b512 refactor: drop redundant hash argument from FetchBlock (Sjors Provoost)
8d1a3e6498de6087501969a9d243b0697ca3fe97 rpc: allow empty JSON object result (Sjors Provoost)
bfbf91d0b2004dde358253ac174982f784b43b59 test: fancier Python for getblockfrompeer (Sjors Provoost)
Pull request description:
Followups from #20295.
ACKs for top commit:
jonatack:
ACK 923312fbf6a89efde1739da0b7209694d4f892ba :package:
fjahr:
tested ACK 923312fbf6a89efde1739da0b7209694d4f892ba
Tree-SHA512: da9eca76e302e249409c9d7f0d16cca668ed981e2ab6ca2d1743dad0d830b94b1bc5ffb9028a00764b863201945c273cc8f4409a4c9ca3817830007dffa2bc20
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
|
|
|
|
|
|
Co-authored-by: John Newbery <john@johnnewbery.com>
|
|
-BEGIN VERIFY SCRIPT-
git grep -l CAddrMan src/ test/ | xargs sed -i 's/CAddrMan/AddrMan/g'
-END VERIFY SCRIPT-
|
|
scheduler threads
703b1e612a4bd4521e20ae21eb8fb7c19f4ef942 Close minor startup race between main and scheduler threads (Larry Ruane)
Pull request description:
This is a low-priority bug fix. The scheduler thread runs `CheckForStaleTipAndEvictPeers()` every 45 seconds (EXTRA_PEER_CHECK_INTERVAL). If its first run happens before the active chain is set up (`CChain::SetTip()`), `bitcoind` will assert:
```
(...)
2021-07-28T22:16:49Z init message: Loading block index…
bitcoind: validation.cpp:4968: CChainState& ChainstateManager::ActiveChainstate() const: Assertion `m_active_chainstate' failed.
Aborted (core dumped)
```
I ran into this while using the debugger to investigate an unrelated problem. Single-stepping through threads with a debugger can cause the relative thread execution timing to be very different than usual. I don't think any automated tests are needed for this PR. I'll give reproduction steps in the next PR comment.
ACKs for top commit:
MarcoFalke:
cr ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942
tryphe:
tested ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942
0xB10C:
ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942
glozow:
code review ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942 - it makes sense to me to start peerman's background tasks here, after `chainstate->LoadChainTip()` and `node.connman->Start()` have been called.
Tree-SHA512: 9316ad768cba3b171f62e2eb400e3790af66c47d1886d7965edb38d9710fc8c8f8e4fb38232811c9346732ce311d39f740c5c2aaf5f6ca390ddc48c51a8d633b
|
|
Don't schedule class PeerManagerImpl's background tasks from its
constructor, but instead do that from a separate method,
StartScheduledTasks(), that can be called later at the end of startup,
after other things, such as the active chain, are initialzed.
|
|
|
|
Includes logging improvements by Vasil Dimov and John Newbery.
|
|
Callers of the external RelayTransactions() no longer need to lock cs_main.
|
|
|
|
|
|
680eb56d828ce358b4e000c140f5b247ff5e6179 [net processing] Don't pass CConnman to RelayTransactions (John Newbery)
a38a4e8f039dfabfd9435f3a63f1a9b56de086d6 [net processing] Move RelayTransaction into PeerManager (John Newbery)
Pull request description:
This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
ACKs for top commit:
ajtowns:
ACK 680eb56d828ce358b4e000c140f5b247ff5e6179
Tree-SHA512: 8c93491a4392b6369bb7f090de326a63cd62a088de59026e202f226f64ded50a0cf1a95ed703328860f02a9d2f64d3a87ca1bca9a6075b978bd111d384766235
|
|
0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f Make tx relay data structure use std::chrono types (Pieter Wuille)
55e82881a1503bff146970856c1474a6ea659c94 Make all Poisson delays use std::chrono types (Pieter Wuille)
c733ac4d8a597b1001555b142f488ed9fbecc405 Convert block/header sync timeouts to std::chrono types (Pieter Wuille)
4d98b401fbd821700f7a792b0a4cb52c9b71bc9f Change all ping times to std::chrono types (Pieter Wuille)
Pull request description:
(Picking up #20044. Rebased against master.)
This changes various uses of integers to represent timestamps and durations to `std::chrono` duration types with type-safe conversions, getting rid of various `.count()`, constructors, and conversion factors.
ACKs for top commit:
jnewbery:
utACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
vasild:
ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
MarcoFalke:
re-ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
ajtowns:
utACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f
Tree-SHA512: 2dbd8d53bf82e98f9b4611e61dc14c448e8957d1a02575b837fadfd59f80e98614d0ccf890fc351f960ade76a6fb8051b282e252e81675a8ee753dba8b1d7f57
|
|
Use the local m_connman instead
|
|
We don't mark RelayTransaction as const. Even though it doesn't mutate
PeerManagerImpl state, it _is_ mutating the internal state of a CNode
object, by updating setInventoryTxToSend. In a subsequent commit, that
field will be moved to the Peer object, which is owned by
PeerMangerImpl.
This requires PeerManagerImpl::ReattemptInitialBroadcast() to no longer
be const.
|
|
|
