c6be144c4b774a03a8bcab5a165768cf81e9b06b Remove timedata (stickies-v)
92e72b5d0d49aa395e626c238bc28aba8e4c3d44 [net processing] Move IgnoresIncomingTxs to PeerManagerInfo (dergoegge)
7d9c3ec622d73a98d07ab3cee78751718982a5bc [net processing] Introduce PeerManagerInfo (dergoegge)
ee178dfcc1175e0af8163216c9c024f4bfc97965 Add TimeOffsets helper class (stickies-v)
55361a15d1aa6984051441bce88112000688fb43 [net processing] Use std::chrono for type-safe time offsets (stickies-v)
038fd979effb54ee76ce1b7cf078e920c652326a [net processing] Move nTimeOffset to net_processing (dergoegge)
Pull request description:
[An earlier approach](1d226ae1f9/) in #28956 involved simplifying and refactoring the network-adjusted time calculation logic, but this was eventually [left out](https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1904214370) of the PR to make it easier for reviewers to focus on consensus logic changes.
Since network-adjusted time is now only used for warning/informational purposes, cleaning up the logic (building on @dergoegge's approach in #28956) should be quite straightforward and uncontroversial. The main changes are:
- Previously, we would only calculate the time offset from the first 199 outbound peers that we connected to. This limitation is now removed, and we have a proper rolling calculation. I've reduced the set to 50 outbound peers, which seems plenty.
- Previously, we would automatically use the network-adjusted time if the difference was < 70 mins, and warn the user if the difference was larger than that. Since there is no longer any automated time adjustment, I've changed the warning threshold to ~~20~~ 10 minutes (which is an arbitrary number).
- Previously, a warning would only be raised once, and then never again until node restart. This behaviour is now updated to 1) warn to log for every new outbound peer for as long as we appear out of sync, 2) have the RPC warning toggled on/off whenever we go in/out of sync, and 3) have the GUI warn whenever we are out of sync (again), but limited to 1 messagebox per 60 minutes
- no more globals
- remove the `-maxtimeadjustment` startup arg
Closes#4521
ACKs for top commit:
sr-gi:
Re-ACK [c6be144](c6be144c4b)
achow101:
reACK c6be144c4b774a03a8bcab5a165768cf81e9b06b
dergoegge:
utACK c6be144c4b774a03a8bcab5a165768cf81e9b06b
Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96
It should never be a nullopt when the transaction result is valid -
Assume() this is the case. However, as a belt-and-suspenders just in
case it is nullopt, use an empty list.
This helper class is an alternative to CMedianFilter, but without a
lot of the special logic and exceptions that we needed while it was
still used for consensus.
All `CTransactionRef` have `.GetWitnessHash()` that returns a cached `const Wtxid` (since fac1223a568fa1ad6dd602350598eed278d115e8),
so we don't need to pass transaction refs around with their IDs as they're easy to get from a ref.
07cd510ffe791a4dfc1824c67fb440be780a4e2b [refactor] consolidate invalid MempoolAcceptResult processing (glozow)
9353aa4066a85705154800efa61c5601036be921 [refactor] consolidate valid MempoolAcceptResult processing (glozow)
Pull request description:
Every time we try to `ProcessTransaction` (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in `m_recent_rejects` so we don't try to download/validate it again.
There are 2 current places and at least 1 future place where we need to process `MempoolAcceptResult`:
- In the `ProcessMessage` logic after receiving and validating a tx
- In `ProcessOrphanTx` where we retry orphans
- With #28970, after processing a package of transactions, we should do these updates for each tx in the package.
Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR.
ACKs for top commit:
instagibbs:
ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b
achow101:
ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b
dergoegge:
Code review ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b
TheCharlatan:
ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b
Tree-SHA512: c4e74cb65e4f52882fca52e6682efa5dcf1562d98418454e09be256ffd026caae642a90aa5b9cccaae214be240d6f4be9d87b516953b2ee69a655f16ea569ed9
0a533613fb44207053796fd01a9f4b523a3153d4 docs: add release notes for #27114 (brunoerg)
e6b8f19de9a6d1c477d0bbda18d17794cd81a6f4 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854cc86deb747caea5283c17cf51b6a983 test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d1749f43d7b314aa2784a06af78440170 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c5e14cbe75256eba170e0867f95f360 net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5cc9a0ab1a06a60d09f1b7e1039018e net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb62d987b3e1c3b8bfad7083f0f774b2 net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)
Pull request description:
Revives #17167. It allows whitelisting manual connections. Fixes#9923
Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
- Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
- https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
- https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
- Force-relay/mempool permissions for a node you intentionally connected to.
ACKs for top commit:
achow101:
ACK 0a533613fb44207053796fd01a9f4b523a3153d4
sr-gi:
re-ACK [0a53361](0a533613fb)
pinheadmz:
ACK 0a533613fb44207053796fd01a9f4b523a3153d4
Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
c5b5843d8f10d96f76ee6b95f2b1b1b4ce755f75 test: avoid requesting blocks beyond limited peer threshold (furszy)
2f6a05512fa86d086b2b976c401394571d88bd93 p2p: sync from limited peer, only request blocks below threshold (furszy)
73127722a2d2b5c8da4102284f9d0cf504a2e72d refactor: Make FindNextBlocks friendlier (furszy)
Pull request description:
Even when the node believes it has IBD completed, need to avoid
requesting historical blocks from network-limited peers.
Otherwise, the limited peer will disconnect right away.
The simplest scenario could be a node that gets synced, drops
connections, and stays inactive for a while. Then, once it re-connects
(IBD stays completed), the node tries to fetch all the missing blocks
from any peer, getting disconnected by the limited ones.
Note:
Can verify the behavior by cherry-picking the test commit alone on
master. It will fail there.
ACKs for top commit:
achow101:
ACK c5b5843d8f10d96f76ee6b95f2b1b1b4ce755f75
vasild:
ACK c5b5843d8f10d96f76ee6b95f2b1b1b4ce755f75
mzumsande:
Code Review ACK c5b5843d8f10d96f76ee6b95f2b1b1b4ce755f75
pinheadmz:
ACK c5b5843d8f10d96f76ee6b95f2b1b1b4ce755f75
Tree-SHA512: 9e550698bc6e63cc587b2b988a87d0ab555a8fa188c91c3f33287f8201d77c28b373331845356ad86f17bb21c15950b6466bc1cafd0ce8139d70364cb71c2ad2
Deduplicate code that exists in both tx processing and ProcessOrphanTx.
Additionally, this can be reused in a function that handles multiple
MempoolAcceptResults from package submission.
Deduplicate code that exists in both tx processing and ProcessOrphanTx.
Additionally, this can be reused in a function that handles multiple
MempoolAcceptResults from package submission.
We preemptively perform a block mutation check before further processing
a block message (similar to early sanity checks on other messsage
types). The main reasons for this change are as follows:
- `CBlock::GetHash()` is a foot-gun without a prior mutation check, as
the hash returned only commits to the header but not to the actual
transactions (`CBlock::vtx`) contained in the block.
- We have observed attacks that abused mutated blocks in the past, which
could have been prevented by simply not processing mutated blocks
(e.g. https://github.com/bitcoin/bitcoin/pull/27608).
Requesting historical blocks from network limited peers is a
direct disconnection cause.
The node must only request the blocks who know for sure the
limited peer can provide.
ff9039f6ea876bab2c40a06a93e0dd087f445fa2 Remove GetAdjustedTime (dergoegge)
Pull request description:
This picks up parts of #25908.
The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains.
ACKs for top commit:
naumenkogs:
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2
achow101:
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2
maflcko:
lgtm ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 🤽
stickies-v:
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2
Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
27f260aa6e04f82dad78e9a06d58927546143a27 net: remove now unused global 'g_initial_block_download_completed' (furszy)
aff7d92b1500e2478ce36a7e86ae47df47dda178 test: add coverage for peerman adaptive connections service flags (furszy)
6ed53602ac7c565273b5722de167cb2569a0e381 net: peer manager, dynamically adjust desirable services flag (furszy)
9f36e591c551ec2e58a6496334541bfdae8fdfe5 net: move state dependent peer services flags (furszy)
f9ac96b8d6f4eba23c88f302b22a2c676e351263 net: decouple state independent service flags from desirable ones (furszy)
97df4e38879d2644aeec34c1eef241fed627333e net: store best block tip time inside PeerManager (furszy)
Pull request description:
Derived from #28120 discussion.
By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.
The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections and remained inactive for a duration longer than the limited
peers threshold (the timeframe within which limited peers can provide blocks). In
such cases, upon reconnecting to the network, the node might only establish
connections with limited peers, filling up all available outbound slots. Resulting
in an inability to synchronize the chain (because limited peers will not provide
blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold).
ACKs for top commit:
achow101:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
vasild:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
naumenkogs:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
mzumsande:
Light Code Review ACK 27f260aa6e04f82dad78e9a06d58927546143a27
andrewtoth:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953
Introduces functionality to detect when limited peers connections
are desirable or not. Ensuring that the new connections desirable
services flags stay relevant throughout the software's lifecycle.
(Unlike the previous approach, where once the validation IBD flag
was set, the desirable services flags remained constant forever).
This will let us recover from stalling scenarios where the node had
successfully synced, but subsequently dropped connections and remained
inactive for a duration longer than the limited peers threshold (the
timeframe within which limited peers can provide blocks). Then, upon
reconnection to the network, the node may end up only establishing
connections with limited peers, leading to an inability to synchronize
the chain.
This also fixes a possible limited peers threshold violation during IBD,
when the user configures `-maxtipage` further than the BIP159's limits.
This rule violation could lead to sync delays and, in the worst-case
scenario, trigger the same post-IBD stalling scenario (mentioned above)
but during IBD.
74ebd4d1359edce82a134dfcd3da9840f8d206e2 doc, test: Test and explain service flag handling (Martin Zumsande)
Pull request description:
Service flags received from the peer-to-peer network are handled differently, depending on how we receive them.
If received directly from an outbound peer the flags belong to, they replace existing flags.
If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.
Document that and add test coverage for it.
ACKs for top commit:
achow101:
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
furszy:
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
brunoerg:
utACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
Tree-SHA512: 604adc3304b8e3cb1a10dfd017025c10b029bebd3ef533f96bcb5856fee5d4396a9aed4949908b8e7ef267ad21320d1814dd80f88426330c5c9c2c529c497591
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.
e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615677d226c9f6b3f2b66d833315d40da logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
achow101:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
stickies-v:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
jamesob:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
fa1a38470697796a1a67397a815c8f8256f59224 Move compat.h include from system.h to system.cpp (MarcoFalke)
88887531b704f3943fdb33abbdd5378ecfeee14f Move compat/assumptions.h include to one place that actually needs it (MarcoFalke)
77774110f4dd591a71441851813d59c03c9e3c78 Remove __cplusplus from compat/assumptions.h (MarcoFalke)
faa3d4f1d8ecff444be53215d72e32d71d9ce138 Remove duplicate NDEBUG check from compat/assumptions.h (MarcoFalke)
Pull request description:
Generally, compile-time checks should be close to the code that use them. Especially, since `compat/assumptions.h` is only included in one place, where iwyu suggests to remove it.
Fix all issues:
* The `NDEBUG` check is used in `util/check`, so it is redundant in `compat/assumptions.h`.
* The `__cplusplus` check is redundant with `doc/dependencies.md` (see commit message).
* Add missing `// IWYU pragma: keep` to avoid removing the include by accident.
ACKs for top commit:
achow101:
ACK fa1a38470697796a1a67397a815c8f8256f59224
TheCharlatan:
re-ACK fa1a38470697796a1a67397a815c8f8256f59224
theuni:
ACK fa1a38470697796a1a67397a815c8f8256f59224
Tree-SHA512: f8b6db84be5d8844a2267345c0b1405fcbc39b8b5eeaa24db5b8412a74145fe44cf188b6b0c39cc2b062690ed37ca5b4662473484afe28dbec6469e79961389b
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
43de4d3630274e1287179c86896ed4c2d8b9eff4 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630274e1287179c86896ed4c2d8b9eff4
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
With package validation rules, transactions that fail individually may
sometimes be eligible for reconsideration if submitted as part of a
(different) package. For now, that includes trasactions that failed for
being too low feerate. Add a new TxValidationResult type to distinguish
these failures from others. In the next commits, we will abort package
validation if a tx fails for any other reason. In the future, we will
also decide whether to cache failures in recent_rejects based on this
result (we won't want to reject a package containing a transaction that
was rejected previously for being low feerate).
Package validation also sometimes elects to skip some transactions when
it knows the package will not be submitted in order to quit sooner. Add
a result to specify this situation; we also don't want to cache these
as rejections.
99990194ce26af89308fab5ad0b1c8c26e45f80c Remove WithParams serialization helper (MarcoFalke)
ffffb4af83a47979a0ecc84247bc1167abc2fbf6 scripted-diff: Use ser params operator (MarcoFalke)
fae9054793ff2a15db1a645cce3df749e0de2f39 test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp (MarcoFalke)
Pull request description:
Every serialization parameter struct already has the `SER_PARAMS_OPFUNC`, except for one in the tests.
For consistency, and to remove verbose code, convert the test to `SER_PARAMS_OPFUNC`, and use it everywhere, then remove the `WithParams` helper.
ACKs for top commit:
ajtowns:
reACK 99990194ce26af89308fab5ad0b1c8c26e45f80c
TheCharlatan:
Re-ACK 99990194ce26af89308fab5ad0b1c8c26e45f80c
Tree-SHA512: be9cae4225a502486fe8d552aaf4b2cd2904a9f73cce9d931c6b7c757594ff1982fcc2c30d00d012cd12b0a9531fd609f8bcd7c94b811e965ac087eb8a3589d3
940a49978c70453e1aaf2c4a0bcb382872b844a5 Use type-safe txid types in orphanage (dergoegge)
ed70e6501648466b9ca91a39b83775363e9a726d Introduce types for txids & wtxids (dergoegge)
cdb14d79e809bf7d1612b21b554a9fcfb2ab1c91 [net processing] Use HasWitness over comparing (w)txids (dergoegge)
Pull request description:
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only the orphanage is converted to use these types in this PR)
ACKs for top commit:
achow101:
ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
stickies-v:
ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
hebasto:
ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5, I have reviewed the code and it looks OK.
instagibbs:
re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
BrandonOdiwuor:
re-ACK 940a49978c70453e1aaf2c4a0bcb382872b844a5
glozow:
reACK 940a49978c
Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
