Age | Commit message (Collapse) | Author |
|
fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde test: Add basic addr relay test (MarcoFalke)
fa1793c1c44a3f75a09f9c636467b8274c541bdd net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003f53708b6d5df1ed4e7f8a7c68aa3ac net: Make addr relay mockable (MarcoFalke)
Pull request description:
As usual:
* Switch to std::chrono time to be type-safe and mockable
* Add basic test that relies on mocktime to add code coverage
ACKs for top commit:
naumenkogs:
utACK fa1da3d
promag:
ACK fa1da3d4bfc0511a89f5b19d5a4d89e55ff7ccde (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.
Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
|
|
fa6a00843447d53a5708ea3a629b9150cfe58be2 fuzz: Add process_messages harness (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
Tested ACK fa6a00843447d53a5708ea3a629b9150cfe58be2
Tree-SHA512: 2d8788308c7f45c97ca003378f58a9d51f51265958557a65e5e505b1666b4cb928f0d010622870175090a0ad25e2d10b41f26f4eef14b6ff334a024baa250f8c
|
|
|
|
|
|
|
|
|
|
|
|
|
|
16d6113f4faa901e248adb693d4768a9e5019a16 Refactor message transport packaging (Jonas Schnelli)
Pull request description:
This PR factors out transport packaging logic from `CConnman::PushMessage()`.
It's similar to #16202 (where we refactor deserialization).
This allows implementing a new message transport protocol like BIP324.
ACKs for top commit:
dongcarl:
ACK 16d6113f4faa901e248adb693d4768a9e5019a16 FWIW
ariard:
Code review ACK 16d6113
elichai:
semiACK 16d6113f4faa901e248adb693d4768a9e5019a16 ran functional+unit tests.
MarcoFalke:
ACK 16d6113f4faa901e248adb693d4768a9e5019a16 š
Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1
|
|
|
|
|
|
3c1bc40205a3fcab606e70b0e3c13d68b2860e34 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8eaf1629dd5af8cf7b9717a8e72028251 Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9e62b3d42c85716bfeb06d8f2b507dc Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b667361bf23344149c01594abebd56fdb Add asmap utility which queries a mapping (Gleb Naumenko)
Pull request description:
This PR attempts to solve the problem explained in #16599.
A particular attack which encouraged us to work on this issue is explained here [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)
Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.
A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).
Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.
TODO:
- ~~more unit tests~~
- ~~find a way to test the code without including >1 MB mapping file in the repo.~~
- find a way to check that mapping file is not corrupted (checksum?)
- comments and separate tests for asmap.cpp
- make python code for .map generation public
- figure out asmap distribution (?)
~Interesting corner case: Iām using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~
ACKs for top commit:
laanwj:
re-ACK 3c1bc40205a3fcab606e70b0e3c13d68b2860e34
jamesob:
ACK 3c1bc40205a3fcab606e70b0e3c13d68b2860e34 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
jonatack:
ACK 3c1bc40205a3fcab606e70b0e3c13d68b2860e34
Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
|
|
If ASN bucketing is used, return a corresponding AS
used in bucketing for a given peer.
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
|
|
Initialize CConnman byte counters during construction, so GetTotalBytesRecv()
and GetTotalBytesSent() methods don't return garbage before Start() is called.
Change shouldn't have any effect outside of the GUI. It just fixes a race
condition during a qt test that was observed on travis:
https://travis-ci.org/bitcoin/bitcoin/jobs/634989685
|
|
Instead of using /16 netgroups to bucket nodes in Addrman for connection
diversification, ASN, which better represents an actor in terms
of network-layer infrastructure, is used.
For testing, asmap.raw is used. It represents a minimal
asmap needed for testing purposes.
|
|
1a8f0d5a74d5cc0000456932babf35301f5c1686 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354fc6808b9b13b9e82da1a82f2f50f26a [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
MarcoFalke:
re-ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
|
|
|
|
|
|
b6d2183858975abc961207c125c15791e531edcc Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2 added asserts to check m_addr_known when it's used (User)
090b75c14be6b9ba2efe38a17d141c6e6af575cb p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
|
|
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's:#include <interfaces/chain.h>:#include <banman.h>\n#include <interfaces/chain.h>\n#include <net.h>\n#include <net_processing.h>:' src/node/context.cpp
sed -i 's/namespace interfaces {/class BanMan;\nclass CConnman;\nclass PeerLogicValidation;\n&/' src/node/context.h
sed -i 's/std::unique_ptr<interfaces::Chain> chain/std::unique_ptr<CConnman> connman;\n std::unique_ptr<PeerLogicValidation> peer_logic;\n std::unique_ptr<BanMan> banman;\n &/' src/node/context.h
sed -i '/std::unique_ptr<[^>]\+> \(g_connman\|g_banman\|peerLogic\);/d' src/banman.h src/net.h src/init.cpp
sed -i 's/g_connman/m_context.connman/g' src/interfaces/node.cpp
sed -i 's/g_banman/m_context.banman/g' src/interfaces/node.cpp
sed -i 's/g_connman/m_node.connman/g' src/interfaces/chain.cpp src/test/setup_common.cpp
sed -i 's/g_banman/m_node.banman/g' src/test/setup_common.cpp
sed -i 's/g_connman/node.connman/g' src/init.cpp src/node/transaction.cpp
sed -i 's/g_banman/node.banman/g' src/init.cpp
sed -i 's/peerLogic/node.peer_logic/g' src/init.cpp
sed -i 's/g_connman/g_rpc_node->connman/g' src/rpc/mining.cpp src/rpc/net.cpp src/rpc/rawtransaction.cpp
sed -i 's/g_banman/g_rpc_node->banman/g' src/rpc/net.cpp
sed -i 's/std::shared_ptr<CWallet> wallet =/node.context()->connman = std::move(test.m_node.connman);\n &/' src/qt/test/wallettests.cpp
-END VERIFY SCRIPT-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Recent questions have come up regarding dynamic service registration
(see https://github.com/bitcoin/bitcoin/pull/16442#discussion_r308702676
and the assumeutxo project, which needs to dynamically flip NODE_NETWORK).
While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
|
|
0ba08020c9791f7caf5986ad6490c16a2b66cd83 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1550bc3038dc541c236ac83e0a0e6d5 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027f15c1e4948ea4378954df24e3fee88 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306ea954d7eccdc11502e91a51dab8ec6 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bbe29bf130a2b0c0e85e5bffea107f75 Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd425f8c4e5b6bbb2beecb9c80034fefe1 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba82211ea5988ed7fe21e1b08bc3367e6d4 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b286c42a9b10132b7c2d76712f1a319 [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29fd813e1402b013f402869c25b656d1 Remove unused variable (Suhas Daftuar)
Pull request description:
Transaction relay is optimized for a combination of redundancy/robustness as well as 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).)
ACKs for top commit:
sipa:
ACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83
ajtowns:
ACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
TheBlueMatt:
re-utACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
jnewbery:
utACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83
jamesob:
ACK https://github.com/bitcoin/bitcoin/commit/0ba08020c9791f7caf5986ad6490c16a2b66cd83
Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
|
|
We don't want relay of addr messages to leak information about
these network links.
|
|
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).)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
This helps to distinguish it from CNode::fRelayTxes and avoid bugs like
425278d17bd0edf8a3a7cc81e55016f7fd8e7726
|
|
fa8548c5d1 net: Remove unused unsanitized user agent string CNode::strSubVer (MarcoFalke)
Pull request description:
I fail to see a use case for this unsanitized byte array. In fact this can easily be confused with `cleanSubVer` and be displayed to the user (or logged) by a simple typo that is hard to find in review.
Further reading: https://btcinformation.org/en/developer-reference#version
ACKs for commit fa8548:
promag:
utACK fa8548c, good catch.
practicalswift:
utACK fa8548c5d13957f57f9b1e20e03002600962f7f0
sipa:
utACK fa8548c5d13957f57f9b1e20e03002600962f7f0
Tree-SHA512: 3c3ff1504d1583ad099df9a6aa761458a82ec48a58ef7aaa9b5679a5281dd1b59036ba2932ed708488951a565b669a3083ef70be5a58472ff8677b971162ae2f
|
|
|
|
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.
|