fafb381af8279b2d2ca768df0bf68d7eb036a2f9 Remove mempool global (MarcoFalke)
fa0359c5b30730744aa8a7cd9ffab79ded91041f Remove mempool global from p2p (MarcoFalke)
eeee1104d78eb59a582ee1709ff4ac2c33ee1190 Remove mempool global from init (MarcoFalke)
Pull request description:
This refactor unlocks some nice potential features, such as, but not limited to:
* Removing the fee estimates global (would avoid slightly fragile workarounds such as #18766)
* Making the mempool optional for a "blocksonly" operation mode
Even absent those features, the new code without the global should be easier to maintain, read and write tests for.
ACKs for top commit:
jnewbery:
utACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
hebasto:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9, I have reviewed the code and it looks OK, I agree it can be merged.
darosior:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26
e57980b4738c10344baf136de3e050a3cb958ca5 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f36124972d2364f941de9c3417c65f05b6 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f527631ede1a31c7855151e5c5d91f8f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b4000fed088b8cf7b99674c328d15e1 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443cc16edf974f099b8485e04b3db1b1d7 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d759b13af68acec6d5bfa04aaa24561f8 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b4738c10344baf136de3e050a3cb958ca5, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e08a04807c71eb02104ca9589eaadf1b [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c851e25e1a533bf1d4296f6b55c81332 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142246b5d51056a51dbf4645b31802be4 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
Pull request description:
Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.
ACKs for top commit:
sipa:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
brakmic:
ACK 9847e205bf
luke-jr:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
naumenkogs:
utACK 9847e20
ajtowns:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)
Pull request description:
The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
> the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)
For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).
The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.
ACKs for top commit:
meshcollider:
utACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
laanwj:
Concept and code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
jkczyz:
ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
MarcoFalke:
ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
fjahr:
Code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
fab558612278909df93bdf88f5727b04f13aef0f doc: Use precise permission flags where possible (MarcoFalke)
Pull request description:
Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.
This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.
Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.
ACKs for top commit:
jnewbery:
reACK fab558612278909df93bdf88f5727b04f13aef0f
fjahr:
Code review ACK fab558612278909df93bdf88f5727b04f13aef0f
jonatack:
ACK fab558612278909df93bdf88f5727b04f13aef0f
Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
https://github.com/dashpay/dash/issues/4966
## What was done?
Serialization now depends on the protocol version, and we use an updated
serialization that is spv friendly for the new version
## How Has This Been Tested?
hasn't
## Breaking Changes
this should be backwards compatible, but this likely should get some
release notes.
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
<!--
*** Please remove the following help text before submitting: ***
Provide a general summary of your changes in the Title above
Pull requests without a rationale and clear improvement may be closed
immediately.
Please provide clear motivation for your patch and explain how it
improves
Dash Core user experience or Dash Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always
welcome.
* All other changes should have accompanying unit tests (see
`src/test/`) or
functional tests (see `test/`). Contributors should note which tests
cover
modified code. If no tests exist for a region of modified code, new
tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or
an
explanation of the potential issue as well as reasoning for the way the
bug
was fixed.
* Features are welcome, but might be rejected due to design or scope
issues.
If a feature is based on a lot of dependencies, contributors should
first
consider building the system outside of Dash Core, if possible.
-->
## Issue being fixed or feature implemented
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
minimizing global uses
## What was done?
<!--- Describe your changes in detail -->
Started the deglobalization, a future PR should be done to continue this
deglobalization
## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
## Breaking Changes
<!--- Please describe any breaking changes your code introduces -->
none
## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* llmq: move initialization logic to 'LLMQContext', add unique pointer to NodeContext
* llmq: add aliases to LLMQ globals, expose them to RPC via LLMQContext
* rpc: replace most global invocations with LLMQContext aliases
* rpc: replace quorum RPC global invocations with LLMQContext aliases
* llmq: replace individual global member arguments with context pointer
* llmq: pass aliased context pointer instead of individual globals in tests
* llmq: move BLS worker to LLMQContext, remove global
* llmq: move DKG debug manager to LLMQContext, remove global
* llmq: move DKG session manager to LLMQContext, remove global
* llmq: move quorum share manager to LLMQContext, remove global
* llmq: move quorum signing manager to LLMQContext, remove global
facb71576cd4d2e90fd03e09d29b42fa3d730e8c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
297e09855793feb94c3229ed989bef8b1eac864e Fix doxygen errors (Ben Woosley)
Pull request description:
These are all the remaining errors identified via -Werror=documentation, e.g.:
```
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
```
You can use this to run with `-Wdocumentation` yourself: #14920
ACKs for top commit:
laanwj:
ACK 297e09855793feb94c3229ed989bef8b1eac864e
Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
168b781fe7f3f13b24c52a151f36de4cdd0a340a Continue relaying transactions after they expire from mapRelay (Anthony Towns)
Pull request description:
This change allows peers to request transactions even after they've expired from mapRelay and even if they're not doing mempool requests. This is intended to allow for CPFP of old transactions -- if parent tx P wasn't relayed due to low fees, then a higher fee rate child C is relayed, peers will currently request the parent P, but we prior to this patch, we will not relay it due to it not being in mapRelay.
ACKs for top commit:
MarcoFalke:
re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a (only change is comment fixup)
sdaftuar:
re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a
sipa:
ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a
Tree-SHA512: b206666dd1450cd0a161ae55fd1a7eda2c3d226842ba27d91fe463b551fd924b65b92551b14d6786692e15cf9a9a989666550dfc980b48ab0f8d4ca305bc7762
9a299a59cc8a9ab516e047356c5bc0e93774b557 net: reference instead of copy in BlockConnected range loop (Jon Atack)
Pull request description:
Reference elements in range for loop instead of copying them and
fix Clang `-Wrange-loop-analysis` warning introduced in a029e18
```
net_processing.cpp:1185:25: warning: loop variable 'ptx' of
type 'const std::shared_ptr<const CTransaction>' creates a copy from
type 'const std::shared_ptr<const CTransaction>' [-Wrange-loop-analysis]
for (const auto ptx : pblock->vtx) {
^
net_processing.cpp:1185:14: note: use reference type
'const std::shared_ptr<const CTransaction> &' to prevent copying
for (const auto ptx : pblock->vtx) {
^~~~~~~~~~~~~~~~
1 warning generated.
```
ACKs for top commit:
Empact:
ACK 9a299a59cc
MarcoFalke:
ACK 9a299a59cc8a9ab516e047356c5bc0e93774b557
promag:
ACK 9a299a59cc8a9ab516e047356c5bc0e93774b557.
elichai:
ACK 9a299a59cc8a9ab516e047356c5bc0e93774b557
emilengler:
ACK 9a299a5.
Tree-SHA512: 9284d1b00684877505454a05071212758c8cea083534e2eec09bfc8a9c3059eea811d2008f6a5a678539444f0d5b3134db1bd23da6514b3d3a1440634c8b53be
a029e18c2bf67dd00552b0f4bbc85fa2fa5b973b Use rolling bloom filter of recent block tx's for AlreadyHave() check (Suhas Daftuar)
Pull request description:
In order to determine whether to download or process a relayed transaction, we first try to check whether we already have the transaction -- either in the mempool, in our filter of recently rejected transactions, in our orphan pool, or already confirmed in a block.
Prior to this commit, the heuristic for checking whether a transaction was confirmed in a block is based on whether there's a coin cache entry corresponding to the 0- or 1-index vout of the tx. While that is a quick check, it is very imprecise (eg if those outputs were already spent in another block, we wouldn't detect that the transaction has already been confirmed) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will better capture the case of a transaction which has been confirmed and then fully spent.
This should reduce the bandwidth that we waste by requesting transactions which will not be accepted to the mempool.
To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
ACKs for top commit:
MarcoFalke:
re-ACK a029e18c2b only stylistic and comment fixups 🍴
sipa:
utACK a029e18c2bf67dd00552b0f4bbc85fa2fa5b973b
jonatack:
Code review ACK a029e18c2bf67dd00552b0f4bbc85fa2fa5b973b also built/ran tests and am running bitcoind with mempool debug logging and custom logging. Looked a bit into CRollingBloomFilter and also the mempool median time past checks mentioned above; I don't have a deep understanding of those areas yet but the concept here and changes LGTM. Tests and other optimisations could be added as a follow-up. In favor of seeing this move forward if no major immediate concerns.
Tree-SHA512: 784c9a35bcd3af5db469063ac7d26b4bac430e451e5637a34d8a538c3ffd1433abdd3f06e5584e7a84bfa9e791449e61819397b5a6c7890fa59d78ec3ba507b2
faec689bed7a5b66e2a7675853d10205b933cec8 txmempool: Make entry time type-safe (std::chrono) (MarcoFalke)
faaa1f01daba94b021ca77515266a16d27f0364e util: Add count_seconds time helper (MarcoFalke)
1111170f2f0141084b5b4ed565b2f07eba48599a test: mempool entry time is persisted (MarcoFalke)
Pull request description:
This changes the type of the entry time of txs into the mempool from `int64_t` to `std::chrono::seconds`.
The benefits:
* Documents the type for developers
* Type violations result in compile errors
* After compilation, the two are equivalent (at no run time cost)
ACKs for top commit:
ajtowns:
utACK faec689bed7a5b66e2a7675853d10205b933cec8
laanwj:
ACK faec689bed7a5b66e2a7675853d10205b933cec8
Tree-SHA512: d958e058755d1a1d54cef536a8b30a11cc502b7df0d6ecf84a0ab1d38bc8105a67668a99cd5087a444f6de2421238111c5fca133cdf8e2e2273cb12cb6957845
* fix: move chain activation logic downward to succeed LLMQ initialization
* fix: change order of initialization to reflect dependency
* llmq: pass all global pointers invoked as CDSNotificationInterface arguments
* llmq: pass reference to quorumDKGDebugManager instead of invoking global
* llmq: pass reference to quorumBlockProcessor instead of invoking global
* llmq: pass reference to quorumDKGSessionManager instead of invoking global
* llmq: pass reference to quorumManager instead of invoking global
Co-authored-by: "UdjinM6 <UdjinM6@users.noreply.github.com>"
* llmq: pass reference to quorumSigSharesManager within CSigningManager and networking
* llmq: pass reference to quorumSigSharesManager instead of invoking global
* llmq: pass reference to chainLocksHandler instead of querying global
* llmq: pass reference to quorumInstantSendManager instead of querying global
* trivial: accept argument as const where possible
* style: remove an unneeded const_cast and instead pass by const reference
* style: use const where possible
Co-authored-by: pasta <pasta@dashboost.org>
* coinjoin: make CCoinJoinServer managed pointer, assign CConnman during init
* coinjoin: make CCoinJoinClientQueueManager managed pointer, assign CConnman during init
* sporks: move spork validation logic downwards after CConnman initialization
* sporks: make CSporkManager a pointer, reduce global invocations
* governance: make CGovernanceManager a pointer, reduce global invocations
* llmq: migrate LLMQ subsystem raw pointers to managed pointers
* masternode: make activeMasternodeManager a managed pointer
* masternode: make masternodeSync a managed pointer, assign CConnman during init
* refactor: make instantsend helper functions class members
* fix: send empty CDeterministicMNList if pointer isn't initialized yet
* fix: refactor governance object retrieval logic across node and ui
Update src/interfaces/node.cpp
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* refactor(llmq): substitute memberless class llmq::CLLMQUtils with namespace llmq::utils
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* chore: mark functions internal to `llmq::utils` as `static`
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* feat: switch nTimeFirstMessageReceived from microseconds to seconds
Was acting more like a bool until now, so nothing should change really. Align it with nTimeConnected.
* fix(dkg/net): Drop outdated connections to nodes that became masternodes recently
Such nodes won't be seen as masternodes by RelayInvToParticipants otherwise so no contributions will be sent to them when they are picked as relay members which in its turn may result in other nodes PoSe-punishing us.
We add them via EnsureQuorumConnections+ThreadOpenMasternodeConnections so they are clearly masternode connections and they are dropped regularly which is annoying. But also, we don't want every masternode connection to be a qwatch one, we want only the ones we added via that algo.
0ea5d70b4756f376342417e0019490233cb4a918 Updated comment for the condition where a transaction relay is denied (glowang)
be01449cc8eb7bb97531a967f5d1dcc7b8865d1e Add test for param interaction b/w -blocksonly and -whitelistforcerelay (glowang)
Pull request description:
Related to: #18428
When -blocksonly is turned on, a node would still relay transactions from whitelisted peers. This funcitonality has not been tested.
ACKs for top commit:
MarcoFalke:
ACK 0ea5d70b4756f376342417e0019490233cb4a918
Tree-SHA512: 4e99c88281cb518cc67f5f3be7171a7b413933047b5d24a04bb3ff2210a82e914d69079f64cd5bac9206ec435e21a622c8e69cedbc2ccb39d2328ac5c01668e5
* feat(llmq): Introduce useRotation in LLMQParams
* fix(llmq): Fix IsQuorumRotationEnabled to recognize all dip0024 quorums
* fix(llmq): Do not allow rotation llmqs for `-llmqinstantsend` and non-rotation ones for `-llmqinstantsenddip0024`
* fix(llmq): Unify and fix IsMiningPhase
NOTE: no need for 1 extra block in mining phase for rotation quorums
* chore(llmq): Reduce the number of IsQuorumRotationEnabled calls
* chore(llmq): Improve logging
* feat(llmq): Make `llmq-` threads for rotation quorums distinguishable by quorum index
* fix(llmq): Fix another endless loop in GetQuorumRelayMembers
* throw an error when a llmq type with an incompatible rotation flag is picked for `-llmq...` params
* Add a note about loop conditions
* llmq: Make TransactionRemovedFromMempool the last action for invalid txes, just like we do for orphans with rejected parents
Write to log, send reject msg and (maybe) punish first and only then notify IS about the tx removal. Makes it easier to reason about it when reading logs.
* Remove unused variable
* [refactor] Move tx relay state to separate structure
* [refactor] Change tx_relay structure to be unique_ptr
* Check that tx_relay is initialized before access
* Add comment explaining intended use of m_tx_relay
* Add 2 outbound block-relay-only connections
Transaction relay is primarily optimized for balancing redundancy/robustness
with bandwidth minimization -- as a result transaction relay leaks information
that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of
a given transaction.
(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction
relay; inferring network connectivity from the relay of blocks/block headers is
much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that
are only used for block relay. (In the future, we might consider rotating our
transaction-relay peers to help limit the effects of (a).)
* Don't relay addr messages to block-relay-only peers
We don't want relay of addr messages to leak information about
these network links.
* doc: improve comments relating to block-relay-only peers
* Disconnect peers violating blocks-only mode
If we set fRelay=false in our VERSION message, and a peer sends an INV or TX
message anyway, disconnect. Since we use fRelay=false to minimize bandwidth,
we should not tolerate remaining connected to a peer violating the protocol.
* net_processing. Removed comment + fixed formatting
* Refactoring net_processing, removed duplicated code
* Refactor some bool in a many-arguments function to enum
It's made to avoid possible typos with arguments, because some of them have default values and it's very high probability to make a mistake here.
* Added UI debug option for Outbound
* Fixed data race related to `setInventoryTxToSend`, introduced in `[refactor] Move tx relay state to separate structure`
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
37fe80e6267094f6051ccf9bec0c7f1a6b9e15da Only consider addrv2 peers for relay of non-addrv1 addresses (Pieter Wuille)
83f8821a6f41854edd5c0b11deabba658890cde1 refactor: add IsAddrCompatible() to CNode (Pieter Wuille)
Pull request description:
When selecting peers to relay an address to, only pick addrv2-capable ones if the address cannot be represented in addr(v1).
Without this I expect that propagation of torv3 addresses over the cleartext network will be very hard for a while.
ACKs for top commit:
jonatack:
ACK 37fe80e6267094f6051ccf9bec0c7f1a6b9e15da
vasild:
ACK 37fe80e6267094f6051ccf9bec0c7f1a6b9e15da
Tree-SHA512: 18a854ea43ad473cf89b9c5193b524109d7af75c26f7aa7e26cd72ad0db52f19c8001d566c607a7e6772bc314f770f09b6c3e07282d110c5daea193edc592cd2
1583498fb6781c01ca2f33c09319ed793964c574 Send and require SENDADDRV2 before VERACK (Pieter Wuille)
c5a89196602e43ebb1cdc9cd4f08d153419c13e1 Don't send 'sendaddrv2' to pre-70016 software (Pieter Wuille)
Pull request description:
BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some implementations reject messages they don't know. As a courtesy, don't send it to nodes with a version before 70016, as no software is known to support BIP155 that doesn't announce at least that protocol version number.
Also move the sending of sendaddrv2 earlier (before sending verack), as proposed in https://github.com/bitcoin/bips/pull/1043. This has the side effect that local address broadcast of torv3 will work (as it'll only trigger after we know whether or not the peer supports addrv2).
ACKs for top commit:
MarcoFalke:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
jnewbery:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
jonatack:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
vasild:
ACK 1583498
Tree-SHA512: 3bd5833fa8c8567b6dedd99e4a9b6bb71c127aa66d5284b217503c86d597dc59aa7382c41f3a4bf561bb658b89db81d1a7703a700eef4ffc17cb916660e23a82
