e11b649650 validation: CVerifyDB::VerifyDB: Use locking annotation (Carl Dong)
03f75c42e1 validation: Use existing chain member in CChainState::LoadGenesisBlock (Carl Dong)
5e4af77380 validation: Use existing chain member in CChainState::AcceptBlock (Carl Dong)
fee73347c0 validation: Pass in chain to FindBlockPos+SaveBlockToDisk (Carl Dong)
a9d28bcd8d validation: Use *this in CChainState::ActivateBestChainStep (Carl Dong)
4744efc9ba validation: Pass in chainstate to CTxMemPool::check (Carl Dong)
1fb7b2c595 validation: Use *this in CChainState::InvalidateBlock (Carl Dong)
8cdb2f7e58 validation: Move LoadBlockIndexDB to CChainState (Carl Dong)
8b99efbcc0 validation: Move invalid block handling to CChainState (Carl Dong)
2bdf37fe18 validation: Pass in chainstate to CVerifyDB::VerifyDB (Carl Dong)
31eac50c72 validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics} (Carl Dong)
63e4c7316a validation: Pass in chainstate to ::PruneBlockFilesManual (Carl Dong)
4bada76237 validation: Pass in chainstate to UpdateTip (Carl Dong)
a3ba08ba7d validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFailureFlags} (Carl Dong)
4927c9e699 validation: Remove global ::LoadGenesisBlock (Carl Dong)
9da106be4d validation: Check chain tip is non-null in CheckFinalTx (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #20750 | [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
Note to self:
- [x] Address: https://github.com/bitcoin/bitcoin/pull/20750#discussion_r579400663
ACKs for top commit:
laanwj:
Code review ACK e11b649650
Tree-SHA512: 205a451a741e32f17d5966de289f2f5a3f0817738c0087b70ff4755ddd217b53d01050ed396669bda2b1d216a88d927b9778777f9ff95ab1fe20e59c5f341776
0eaea66e8b Make tx relay data structure use std::chrono types (Pieter Wuille)
55e82881a1 Make all Poisson delays use std::chrono types (Pieter Wuille)
c733ac4d8a Convert block/header sync timeouts to std::chrono types (Pieter Wuille)
4d98b401fb 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 0eaea66e8b
vasild:
ACK 0eaea66e8b
MarcoFalke:
re-ACK 0eaea66e8b, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
ajtowns:
utACK 0eaea66e8b
Tree-SHA512: 2dbd8d53bf82e98f9b4611e61dc14c448e8957d1a02575b837fadfd59f80e98614d0ccf890fc351f960ade76a6fb8051b282e252e81675a8ee753dba8b1d7f57
Collects all the orphan handling globals into a single member var in
net_processing, and ensures access is encapuslated into the interface
functions. Also adds doxygen comments for methods.
All the interesting functionality of AddOrphanTx is already in other
functions, so call those functions directly in the one place that
AddOrphanTx was used.
EraseOrphansFor was called both with and without g_cs_orphans held,
correct that so that it's always called with it already held.
LimitOrphanTxSize was always called with g_cs_orphans held, so
add annotations and don't lock it a second time.
fa55159b9e net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers (MarcoFalke)
Pull request description:
The goal is to avoid local peers (e.g. untrusted peers on the local network or inbound onion peers via a local onion proxy) filling the debug log (and thus the disk).
ACKs for top commit:
practicalswift:
ACK fa55159b9e
vasild:
ACK fa55159b9e
Tree-SHA512: de233bf57334580f9b91f369fafd131d71c5ae25db25b09cc8fa8cbf34c0648f083c52260a6a912238751467e3c3c5f5d2309c145710753058d44a0003f88f4f
e8ae1db864 style-only: Make AcceptToMemoryPool signature readable (Carl Dong)
8f5c100064 style-only: Make CheckSequenceLock signature readable (Carl Dong)
8c824819c8 validation: Use *this in CChainState::LoadMempool (Carl Dong)
0a9a24d8c7 validation: Pass in chainstate to UpdateMempoolForReorg (Carl Dong)
7142018812 validation: Pass in chainstate to CTxMemPool::removeForReorg (Carl Dong)
71734c65dc validation: Pass in chain to ::TestLockPointValidity (Carl Dong)
120aaba9ac tree-wide: Fix erroneous AcceptToMemoryPool replacements (Carl Dong)
417dafc1ee validation: Remove old AcceptToMemoryPool w/o chainstate param (Carl Dong)
3704433c4f scripted-diff: Invoke ::AcceptToMemoryPool with chainstate (Carl Dong)
229bc37b5f validation: Pass in chainstate to ::AcceptToMemoryPool (Carl Dong)
d0da7ea57a validation: Pass in chainstate to ::LoadMempool (Carl Dong)
3a205c43dc validation: Pass in chainstate to AcceptToMemoryPoolWithTime (Carl Dong)
d8a816329c validation: Add chainstate member to MemPoolAccept (Carl Dong)
4c15942b79 validation: Pass in chainstate to ::CheckSequenceLocks (Carl Dong)
577b774d0c validation: Remove old CheckFinalTx w/o chain tip param (Carl Dong)
7031cf89db scripted-diff: Invoke ::CheckFinalTx with chain tip (Carl Dong)
d015eaa550 validation: Pass in chain tip to ::CheckFinalTx (Carl Dong)
252b489c9f validation: Pass in coins tip to CheckInputsFromMempoolAndCache (Carl Dong)
73a6d2b7be validation: Pass in chainstate to IsCurrentForFeeEstimation (Carl Dong)
d1f932b0b0 validation: Pass in coins cache to ::LimitMempoolSize (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
glozow:
reACK e8ae1db864 via `git range-diff 15f0042...e8ae1db`, only change is fixing ATMP call from conflict
MarcoFalke:
ACK e8ae1db864📣
Tree-SHA512: 6af50f04940a69c5c3d3796a24f32f963fa02503cdc1155cc11fff832a99172b407cd163a19793080a5af98580f051b48195b62ec4a797ba2763b4883174153d
3e68efa615 [net] Move checks from GetLocalAddrForPeer to caller (John Newbery)
d21d2b264c [net] Change AdvertiseLocal to GetLocalAddrForPeer (John Newbery)
Pull request description:
This is the first part of #21186. It slightly disentangles addr handling in net/net_processing by making it explicit that net_processing is responsible for pushing addr records into `vAddrToSend`.
ACKs for top commit:
MarcoFalke:
re-ACK 3e68efa615🍅
Tree-SHA512: 9af50c41f5a977e2e277f24a589db38e2980b353401def5e74b108ac5f493d9b5d6b1b8bf15323a4d66321495f04bc271450fcef7aa7d1c095f051a4f8e9b15f
GetLocalAddrForPeer() is only called in one place. The checks inside that
function make more sense to be carried out be the caller:
- fSuccessfullyConnected is already checked at the top of
SendMessages(), so must be true when we call GetLocalAddrForPeer()
- fListen can go into the conditional before GetLocalAddrForPeer() is
called.
Gossiping addresses to peers is the responsibility of net processing.
Change AdvertiseLocal() in net to just return an (optional) address
for net processing to advertise. Update function name to reflect
new responsibility.
fafddfadda scripted-diff: Remove shadowing lock annotations (MarcoFalke)
Pull request description:
Would be good to not redundantly copy the lock annotation from the class declaration to the member implementation. Otherwise it may not result in a compile failure if a new lock requirement is added to the member implementation, but not the class declaration.
ACKs for top commit:
amitiuttarwar:
ACK `fafddfadda`, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :)
hebasto:
ACK fafddfadda
jonatack:
Light utACK fafddfadda verified that the removed annotations in the definitions correspond to those in their respective declarations
Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2
Can be reviewed with --word-diff-regex=.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/(PeerManagerImpl::.*\)).*LOCKS_.*\)/\1/g' ./src/net_processing.cpp
-END VERIFY SCRIPT-
6452190841 net_processing: simplify MaybeSetPeerAsAnnouncingHeaderAndIDs args (Anthony Towns)
39c2a69bc2 net_processing: move MaybeSetPeerAsAnnouncingHeadersAndIDs into PeerManagerImpl (Anthony Towns)
7b7117efd0 net_processing: simplify ProcessGetData and FindTxForGetData args (Anthony Towns)
34207b9004 net_processing: move FindTxForGetData and ProcessGetData to PeerManagerImpl (Anthony Towns)
d44084883a net_processing: simplify PeerManageImpl method args (Anthony Towns)
a490f0a056 net_processing: move MarkBlockAs*, TipMayBeStale, FindNextBlocksToDL to PeerManagerImpl (Anthony Towns)
052d9bc7e5 net_processing: simplify AlreadyHaveTx args (Anthony Towns)
eeac506250 net_processing: move AlreadyHaveTx into PeerManageImpl (Anthony Towns)
9781c08a33 net_processing: move some globals into PeerManagerImpl (Anthony Towns)
Pull request description:
Turns some globals into member variables, and simplifies the parameter list for some of net_processing's internal functions. Mostly just serves as a code cleanup at this point.
ACKs for top commit:
jnewbery:
Code review ACK 6452190841
ariard:
Code Review ACK 6452190, changes are pretty straightforward.
MarcoFalke:
Concept ACK 6452190841 I have not reviewed this, but I left a comment 🐡
Tree-SHA512: 381361f9dbfeb851a5522ead3165ce1447a0f212ddea4b483aa38975559ee5ed03a4ba69c24fd69f36847a1eddfef05785f5cbb2fcec5fe50f8b336e8047c3b1
53e716ea11 [refactor] improve style for touched code (gzhao408)
174cb5330a [refactor] const ATMPArgs and non-const Workspace (gzhao408)
f82baf0762 [refactor] return MempoolAcceptResult (gzhao408)
9db10a5506 [refactor] clean up logic in testmempoolaccept (gzhao408)
Pull request description:
This is the first 4 commits of #20833, and does refactoring only. It should be relatively simple to review, and offers a few nice things:
- It makes accessing values that don't make sense (e.g. fee) when the tx is invalid an error.
- Returning `MempoolAcceptResult` from ATMP makes the interface cleaner. The caller can get a const instead of passing in a mutable "out" param.
- We don't have to be iterating through a bunch of lists for package validation, we can just return a `std::vector<MempoolAcceptResult>`.
- We don't have to refactor all ATMP call sites again if/when we want to return more stuff from it.
ACKs for top commit:
MarcoFalke:
ACK 53e716ea11💿
jnewbery:
Code review ACK 53e716ea11
ariard:
Code Review ACK 53e716e, I did tweak a bit the touched paths to see if we had good test coverage. Didn't find holes.
Tree-SHA512: fa6ec324a08ad9e6e55948615cda324cba176255708bf0a0a0f37cedb7a75311aa334ac6f223be7d8df3c7379502b1081102b9589f9a9afa1713ad3d9ab3c24f
3ddbf22ed1 util: Disallow negative mocktime (MarcoFalke)
f5f2f97168 net: Avoid UBSan warning in ProcessMessage(...) (practicalswift)
Pull request description:
Avoid UBSan warning in `ProcessMessage(...)`.
Context: https://github.com/bitcoin/bitcoin/pull/20380#issuecomment-770427182 (thanks Crypt-iQ!)
ACKs for top commit:
MarcoFalke:
re-ACK 3ddbf22ed1 only change is adding patch written by me
ajtowns:
ACK 3ddbf22ed1 -- code review only
Tree-SHA512: e8d7af0457ca86872b75a4e406c0a93aafd841c2962e244e147e748cc7ca118c56be0fdafe53765f4b291410030b2c3cc8f76f733b37a955d34fc885ab6037b9
This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
67c9a83df1 style-only: Remove redundant sentence in ActivateBestChain comment (Carl Dong)
b8e95658d5 style-only: Make TestBlockValidity signature readable (Carl Dong)
0cdad75390 validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock (Carl Dong)
ea4fed9021 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (Carl Dong)
e0dc305727 validation: Move LoadExternalBlockFile to CChainState (Carl Dong)
5f8cd7b3a5 validation: Remove global ::ActivateBestChain (Carl Dong)
2a696472a1 validation: Pass in chainstate to ::NotifyHeaderTip (Carl Dong)
9c300cc8b3 validation: Pass in chainstate to TestBlockValidity (Carl Dong)
0e17c833cd validation: Make CChainState.m_blockman public (Carl Dong)
d363d06bf7 validation: Pass in blockman to ContextualCheckBlockHeader (Carl Dong)
f11d11600d validation: Move GetLastCheckpoint to BlockManager (Carl Dong)
e4b95eefbc validation: Move GetSpendHeight to BlockManager (Carl Dong)
b026e318c3 validation: Move FindForkInGlobalIndex to BlockManager (Carl Dong)
3664a150ac validation: Remove global LookupBlockIndex (Carl Dong)
eae54e6e60 scripted-diff: Use BlockManager::LookupBlockIndex (Carl Dong)
15d20f40e1 validation: Move LookupBlockIndex to BlockManager (Carl Dong)
f92dc6557a validation: Guard the active_chainstate with cs_main (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jnewbery:
utACK 67c9a83df1
laanwj:
re-ACK 67c9a83df1
ryanofsky:
Code review ACK 67c9a83df1. Changes since last review:
Tree-SHA512: 8744aba2dd57a40cd2fedca809b0fe24d771bc60da1bffde89601999384aa0df428057a86644a3f72fbeedbc8b04db6c4fd264ea0db2e73c279e5acc6d056cbf
nSyncStarted, mapBlockSource, g_wtxid_relay_peers,
g_outbound_peers_with_protect_from_disconnect were all only used by
PeerManagerImpl methods already.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
FindForkInGlobalIndex only acts on BlockManager.
Note to reviewers: Since FindForkInGlobalIndex is always called with
::ChainActive() as its first parameter, it is possible to move
FindForkInGlobalIndex to CChainState and remove this const CChain&
parameter to instead use m_chain. However, it seems like the original
intention was for FindForkInGlobalIndex to work with _any_ chain, not
just the current active chain. Let me know if this should be changed.
