| Age | Commit message (Collapse) | Author |
|---|
|
|
|
peers.dat is empty
fe3e993968d6b46777d5a16a662cd22790ddf5bb [p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. (Dhruv Mehta)
Pull request description:
Closes #19795
Before PR: If `peers.dat` is empty and `-dnsseed=0`, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay.
After PR: There's no 60 second delay.
To reproduce:
`rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0` without and with patch code
Other changes in the PR:
- `-fixedseeds` command line argument added: `-dnsseed=0 -fixedseeds=0 -addnode=X` provides a trusted peer only setup. `-dnsseed=0 -fixedseeds=0` allows for a `addnode` RPC to add a trusted peer without falling back to hardcoded seeds.
ACKs for top commit:
LarryRuane:
re-ACK fe3e993968d6b46777d5a16a662cd22790ddf5bb
laanwj:
re-ACK fe3e993968d6b46777d5a16a662cd22790ddf5bb
Tree-SHA512: 79449bf4e83a315be6dbac9bdd226de89d2a3f7f76d9c5640a2cb3572866e6b0e8ed67e65674c9824054cf13119dc01c7e1a33848daac6b6c34dbc158b6dba8f
|
|
Add -fixedseeds arg.
|
|
615ba0eb96cf131364c1ceca9d3dedf006fa1e1c test: add Sock unit tests (Vasil Dimov)
7bd21ce1efc363b3e8ea1d51dd1410ccd66820cb style: rename hSocket to sock (Vasil Dimov)
04ae8469049e1f14585aabfb618ae522150240a7 net: use Sock in InterruptibleRecv() and Socks5() (Vasil Dimov)
ba9d73268f9585d4b9254adcf54708f88222798b net: add RAII socket and use it instead of bare SOCKET (Vasil Dimov)
dec9b5e850c6aad989e814aea5b630b36f55d580 net: move CloseSocket() from netbase to util/sock (Vasil Dimov)
aa17a44551c03b00a47854438afe9f2f89b6ea74 net: move MillisToTimeval() from netbase to util/time (Vasil Dimov)
Pull request description:
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
ACKs for top commit:
laanwj:
Re-ACK 615ba0eb96cf131364c1ceca9d3dedf006fa1e1c
jonatack:
re-ACK 615ba0eb96cf131364c1ceca9d3dedf006fa1e1c
Tree-SHA512: 3003e6bc0259295ca0265ccdeb1522ee25b4abe66d32e6ceaa51b55e0a999df7ddee765f86ce558a788c1953ee2009bfa149b09d494593f7d799c0d7d930bee8
|
|
Use the `Sock` class instead of `SOCKET` for `InterruptibleRecv()` and
`Socks5()`.
This way the `Socks5()` function can be tested by giving it a mocked
instance of a socket.
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
|
|
Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.
In addition, the new `Sock` class has a `Send()`, `Recv()` and `Wait()`
methods that can be overridden by unit tests to mock the socket
operations.
The `Wait()` method also hides the
`#ifdef USE_POLL poll() #else select() #endif` technique from higher
level code.
|
|
Move `CloseSocket()` (and `NetworkErrorString()` which it uses) from
`netbase.{h,cpp}` to newly added `src/util/sock.{h,cpp}`.
This is necessary in order to use `CloseSocket()` from a newly
introduced Sock class (which will live in `src/util/sock.{h,cpp}`).
`sock.{h,cpp}` cannot depend on netbase because netbase will depend
on it.
|
|
bff7c66e67aa2f18ef70139338643656a54444fe Add documentation to contrib folder (Troy Giorshev)
381f77be858d7417209b6de0b7cd23cb7eb99261 Add Message Capture Test (Troy Giorshev)
e4f378a505922c0f544b4cfbfdb169e884e02be9 Add capture parser (Troy Giorshev)
4d1a582549bc982d55e24585b0ba06f92f21e9da Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97bec09dd5fcc043d8659d8ec5dfb87c2 Add CaptureMessage (Troy Giorshev)
dbf779d5deb04f55c6e8493ce4e12ed4628638f3 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 bff7c66e67aa2f18ef70139338643656a54444fe only some minor changes: 👚
jnewbery:
utACK bff7c66e67aa2f18ef70139338643656a54444fe
theStack:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
|
|
|
|
|
|
|
|
2a39ccf1334ef3c48c6f9969a0fc916b9e10aae1 Add include for std::bind. (sinetek)
Pull request description:
Hi, this patch adds in <functional> because the GUI code makes use of std::bind.
That's all.
ACKs for top commit:
jonasschnelli:
utACK 2a39ccf1334ef3c48c6f9969a0fc916b9e10aae1
Tree-SHA512: fb5ac07d9cd5d006182b52857b289a9926362a2f1bfa4f7f1c78a088670e2ccf39ca28214781df82e8de3909fa3e69685fe1124a7e3ead758575839f5f2277a9
|
|
These calls are toggled by a debug-only "capturemessages" flag. Default
disabled.
|
|
This commit adds the CaptureMessage function. This will later be called
when any message is sent or received. The capture directory is fixed,
in a new folder "message_capture" in the datadir. Peers will then have
their own subfolders, named with their IP address and port, replacing
colons with underscores to keep compatibility with Windows. Inside,
received and sent messages will be captured into two binary files,
msgs_recv.dat and msgs_sent.dat.
e.g.
message_capture/203.0.113.7_56072/msgs_recv.dat
message_capture/203.0.113.7_56072/msgs_sent.dat
The format has been designed as to result in a minimal performance
impact. A parsing script is added in a later commit.
|
|
Also clean up and better comment the function. InactivityChecks() uses a
mixture of (non-mockable) system time and mockable time. Make sure
that's well documented.
Despite being marked as const in CConnman before this commit, the
function did mutate the state of the passed in CNode, which is contained
in vNodes, which is a member of CConnman. To make the function truly
const in CConnman and all its data, instead make InactivityChecks() a
pure function, return whether the peer should be disconnected, and let
the calling function (SocketHandler()) update the CNode object. Also
make the CNode& argument const.
|
|
This brings PushMessage and ProcessMessages further in line with the
style guide by fixing their if statements.
LogMessage is later called, inside an if statement, inside both of these
methods.
|
|
|
|
|
|
functional tests.
b4dd2ef8009703b81235e2d9a2a736a3a5e8152f [test] Test the add_outbound_p2p_connection functionality (Amiti Uttarwar)
602e69e4278f0ed25c65fb568ab395e4c7ca9ceb [test] P2PBlocksOnly - Test block-relay-only connections. (Amiti Uttarwar)
8bb6beacb19864b1fca766b3e153349a31dc0459 [test/refactor] P2PBlocksOnly - Extract transaction violation test into helper. (Amiti Uttarwar)
99791e7560d40ad094eaa73e0be3987581338e2d [test/refactor] P2PBlocksOnly - simplify transaction creation using blocktool helper. (Amiti Uttarwar)
3997ab915451a702eed2153a0727b0a78c0450ac [test] Add test framework support to create outbound connections. (Amiti Uttarwar)
5bc04e8837c0452923cebd1b823a85e5c4dcdfa6 [rpc/net] Introduce addconnection to test outbounds & blockrelay (Amiti Uttarwar)
Pull request description:
The existing functional test framework uses the `addnode` RPC to spin up manual connections between bitcoind nodes. This limits our ability to add integration tests for our networking code, which often executes different code paths for different connection types.
**This PR enables creating `outbound` & `block-relay-only` P2P connections in the functional tests.** This allows us to increase our p2p test coverage, since we can now verify expectations around these connection types.
This builds out the [prototype](https://github.com/bitcoin/bitcoin/issues/14210#issuecomment-527421978) proposed by ajtowns in #14210. 🙌🏽
An overview of this branch:
- introduces a new test-only RPC function `addconnection` which initiates opening an `outbound` or `block-relay-only` connection. (conceptually similar to `addnode` but for different connection types & restricted to regtest)
- adds `test_framework` support so a mininode can open an `outbound`/`block-relay-only` connection to a `P2PInterface`/`P2PConnection`.
- updates `p2p_blocksonly` tests to create a `block-relay-only` connection & verify expectations around transaction relay.
- introduces `p2p_add_connections` test that checks the behaviors of the newly introduced `add_outbound_p2p_connection` test framework function.
With these changes, there are many more behaviors that we can add integration tests for. The blocksonly updates is just one example.
Huge props to ajtowns for conceiving the approach & providing me feedback as I've built out this branch. Also thank you to jnewbery for lots of thoughtful input along the way.
ACKs for top commit:
troygiorshev:
reACK b4dd2ef8009703b81235e2d9a2a736a3a5e8152f
jnewbery:
utACK b4dd2ef8009703b81235e2d9a2a736a3a5e8152f
MarcoFalke:
Approach ACK b4dd2ef8009703b81235e2d9a2a736a3a5e8152f 🍢
Tree-SHA512: d1cba768c19c9c80e6a38b1c340cc86a90701b14772c4a0791c458f9097f6a4574b4a4acc7d13d6790c7b1f1f197e2c3d87996270f177402145f084ef8519a6b
|
|
fad1f0fd33e5e7a65b702237c7ca8e1b694852d2 net: Remove unused cs_feeFilter (MarcoFalke)
Pull request description:
A `RecursiveMutex` is overkill for setting or reading a plain integer. Even a `Mutex` is overkill, when a plain `std::atomic` can be used.
This removes 11 lines of code. Also, it is cutting down on the number of locks put on the stack at the same time, which complicates review looking out for potential lock contention.
ACKs for top commit:
jnewbery:
utACK fad1f0fd33e5e7a65b702237c7ca8e1b694852d2
practicalswift:
cr ACK fad1f0fd33e5e7a65b702237c7ca8e1b694852d2: patch looks correct
Tree-SHA512: 647f9b954fbf52e138d3e710937eb9131b390fef0deae03fd6a162d5a18b9f194010800bbddc8f89208d91be2802dff11c3884d04b3dd233865abd12aa3cde06
|
|
faecb74562d012a336837d3b39572c235ad2eb9d Expose integral m_conn_type in CNodeStats, remove m_conn_type_string (Jon Atack)
Pull request description:
Currently, strings are stored for what are actually integral (strong) enum types. This is fine, because the strings are only used as-is for the debug log and RPC. However, it complicates using them in the GUI. User facing strings in the GUI should be translated and only string literals can be picked up for translation, not runtime `std::string`s.
Fix that by removing the `std::string` members and replace them by strong enum integral types.
ACKs for top commit:
jonatack:
Code review ACK faecb74562d012a336837d3b39572c235ad2eb9d
theStack:
Code review ACK faecb74562d012a336837d3b39572c235ad2eb9d 🌲
Tree-SHA512: 24df2bd0645432060e393eb44b8abaf20fe296457d07a867b0e735c3e2e75af7b03fc6bfeca734ec33ab816a7c8e1f8591a5ec342f3afe3098a4e41f5c2cfebb
|
|
Add a new RPC endpoint to enable opening outbound connections from
the tests. The functional test framework currently uses the addnode RPC, which
has different behavior than general outbound peers. These changes enable
creating both full-relay and block-relay-only connections. The new RPC
endpoint calls through to a newly introduced AddConnection method on
CConnman that ensures we stay within the allocated max.
|
|
This commit does not change behavior.
|
|
|
|
|
|
Also, add lock annotation to SendMessages
Can be reviewed with "--word-diff-regex=."
|
|
const
31b136e5802e1b1e5f9a9589736afe0652f34da2 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee4c7f98d9c1fac5ed7576b96374d4e9 Don't declare de facto const member functions as non-const (practicalswift)
Pull request description:
_Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_
Changes in this PR:
* Don't declare de facto const member functions as non-const
* Don't declare de facto const reference variables as non-const
Awards for finding candidates for the above changes go to:
* `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html))
* `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/))
See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`.
ACKs for top commit:
ajtowns:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
jonatack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
theStack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 :snowflake:
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
|
|
378aedc45248cea82d9a3e6dc1038d6828008a76 [net] Add cs_vSend lock annotations (John Newbery)
673254515a2f97e53dd8c7335c836b083ba7e31a [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery)
Pull request description:
RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend.
Also correctly annotate the CNode data members that are guarded by cs_vSend.
This is a simpler alternative to #19673.
ACKs for top commit:
jnewbery:
ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs.
troygiorshev:
ACK 378aedc45248cea82d9a3e6dc1038d6828008a76
theStack:
re-ACK 378aedc45248cea82d9a3e6dc1038d6828008a76
MarcoFalke:
review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌
Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07
|
|
|
|
|
|
getter, unit tests
86c495223f048e5ca2cf0d8730af7db3b76f7aba net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack)
6609eb8cb50fe92c7317b5db9e72d4333b3aab1b net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack)
993d1ecd191a7d9161082d4026f020cbf00835bb test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack)
Pull request description:
The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in #20197:
- asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary
- fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor
- drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status
- adds a public getter `IsInboundOnion()` that also allows unit testing it
- adds unit test coverage
ACKs for top commit:
sipa:
utACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
LarryRuane:
ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
vasild:
ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
MarcoFalke:
review ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba 🐍
Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204
|
|
|
|
e262a19b0ba07d8d2334551f49ca1577ab2999fc gui: display network in peer details (Jon Atack)
913695307360d3582f74ffe225cb122b2e48023d gui: rename peer tab column headers, initialize in .h (Hennadii Stepanov)
05c08c696a1bc45ab28abe6f82bebbdbd6239c23 gui: add network column in peers tab/window (Jon Atack)
e0e55060bf697fe8b9ed43ff62383a0451e9c0ce gui: fix broken doxygen formatting in src/qt/guiutil.h (Jon Atack)
0d5613f9ded846aa20a95c757672e9560c664c4c gui: create GUIUtil::NetworkToQString() utility function (Jon Atack)
af9103cc792e17f35249f1d4cb30f0d6958ceb75 net, rpc: change CNodeStats::m_network from string to Network (Jon Atack)
Pull request description:
and rename peers window column headers from NodeId and Node/Service to Peer Id and Address.
ACKs for top commit:
laanwj:
ACK e262a19b0ba07d8d2334551f49ca1577ab2999fc
Tree-SHA512: 709c2a805c109c2dd033aca7b6b6dc94ebe2ce7a0168c71249e1e661c9c57d1f1c781a5b9ccf3b776bedeb83ae2fb5c505637337c45b1eb9a418cb1693a89761
|
|
|
|
|
|
|
|
Also rename to m_continuation_block to better communicate meaning.
|
|
|
|
and drop an unneeded check in CNode::ConnectedThroughNetwork()
|
|
default ports
010eed3ce03cf4fc622a48f40fc4d589383f7a44 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas)
Pull request description:
Closes #5150.
This was mostly copied from #5285 by sulks, who has since quit GitHub.
The issue has remained open for 6 years, but the extra explanation still seems useful.
ACKs for top commit:
laanwj:
re-ACK 010eed3ce03cf4fc622a48f40fc4d589383f7a44
Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
|
|
ea36a453e3d06657679459e1168347648fa7c5c0 [net] Make p2p recv buffer timeout 20 minutes for all peers (John Newbery)
Pull request description:
The timeout interval for the send and recv buffers was changed from 90
minutes to 20 minutes in commit f1920e86 in 2013, except for peers that
did not support the pong message (where the recv buffer timeout remained
at 90 minutes). A few observations:
- for peers that support BIP 31 (pong messages), this recv buffer
timeout is almost redundant with the ping timeout. We send a ping
message every two minutes, and set a timeout of twenty minutes to
receive the pong response. If the recv buffer was really timing out,
then the pong response would also time out.
- BIP 31 is supported by all nodes of p2p version 60000 and higher, and
has been in widespread use since 2013. I'd be very surprised if there
are many nodes on the network that don't support pong messages.
- The recv buffer timeout is not specified in any p2p BIP. We're free to
set it at any value we want.
- A peer that doesn't support BIP 31 and hasn't sent any message to us
at all in 90 minutes is unlikely to be useful for us, and is more likely
to be evicted AttemptToEvictConnection() since it'll have the worst
possible ping time and isn't providing blocks/transactions.
Therefore, we remove this check, and set the recv buffer timeout to 20
minutes for all peers. This removes the final p2p version dependent
logic from the net layer, so all p2p version data can move into the
net_processing layer.
Alternative approaches:
- Set the recv buffer timeout to 90 minutes for all peers. This almost
wouldn't be a behaviour change at all (pre-BIP 31 peers would still
have the same recv buffer timeout, and we can't ever reach a recv buffer
timeout higher than 21 minutes for post-BIP31 peers, because the pong
timeout would be hit first).
- Stop supporting peers that don't support BIP 31. BIP 31 has been in
use since 2012, and implementing it is trivial.
ACKs for top commit:
MarcoFalke:
review ACK ea36a453e3d06657679459e1168347648fa7c5c0
promag:
Code review ACK ea36a453e3d06657679459e1168347648fa7c5c0.
practicalswift:
cr ACK ea36a453e3d06657679459e1168347648fa7c5c0: patch looks correct
ajtowns:
ACK ea36a453e3d06657679459e1168347648fa7c5c0
sipa:
utACK ea36a453e3d06657679459e1168347648fa7c5c0
jonatack:
Code review ACK ea36a453e3d06657679459e1168347648fa7c5c0
Tree-SHA512: df290bb32d2b5d9e59a0125bb215baa92787f9d01542a7437245f1c478c7f9b9831e5f170d3cd0db2811e1b11b857b3e8b2e03376476b8302148e480d81aab19
|
|
|
|
fee88237e03c21bf81f21098e6b89ecfa5327cee Assert eviction at >= 29 candidates. Assert non-eviction at <= 20 candidates. (practicalswift)
685c428de0fb63ca6ec1419bb112f07d27bcdf14 test: Add unit testing of node eviction logic (practicalswift)
ed73f8cee0d7b7facbd2e8dde24a237f20c48c0c net: Move eviction node selection logic to SelectNodeToEvict(...) (practicalswift)
Pull request description:
Add unit testing of node eviction logic.
Closes #19966.
ACKs for top commit:
jonatack:
ACK fee88237e03c21bf81f21098e6b89ecfa5327cee
MarcoFalke:
ACK fee88237e03c21bf81f21098e6b89ecfa5327cee 🐼
Tree-SHA512: 0827c35609122ca42bfabb17feaaee35c191ab4dc2e428282af425a6c176eaeaff2789b74a4f7eb4ca6b8cb10603068e90ca800e6ef3bc3904d50e76616f7a2b
|
|
|
|
fa86217e97234aac6f815a6768afc1b87b8b2ae8 doc: Move add relay comment in net to correct place (MarcoFalke)
Pull request description:
The comment was previously attached to `m_addr_known`, but now it is attached to `id`, which is wrong.
Fix that by moving the comment to `RelayAddrsWithConn`.
ACKs for top commit:
practicalswift:
cr ACK fa86217e97234aac6f815a6768afc1b87b8b2ae8: patch looks correct
jnewbery:
ACK fa86217e97
theStack:
Code review ACK fa86217e97234aac6f815a6768afc1b87b8b2ae8 🌳
Tree-SHA512: ec3d5f1996aded38947d2a5fd0bb63539e88f83964cd3254984002edfd51abb4dde813c7c81619a8a3a5c55b7e9ae83c8c5be8ad6c84b4593ed3bbf463fe8979
|
|
Can be reviewed with
--ignore-all-space --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
The timeout interval for the send and recv buffers was changed from 90
minutes to 20 minutes in commit f1920e86 in 2013, except for peers that
did not support the pong message (where the recv buffer timeout remained
at 90 minutes). A few observations:
- for peers that support BIP 31 (pong messages), this recv buffer
timeout is almost redundant with the ping timeout. We send a ping
message every two minutes, and set a timeout of twenty minutes to
receive the pong response. If the recv buffer was really timing out,
then the pong response would also time out.
- BIP 31 is supported by all nodes of p2p version 60000 and higher, and
has been in widespread use since 2013. I'd be very surprised if there
are many nodes on the network that don't support pong messages.
- The recv buffer timeout is not specified in any p2p BIP. We're free to
set it at any value we want.
- A peer that doesn't support BIP 31 and hasn't sent any message to us
at all in 90 minutes is unlikely to be useful for us, and is more likely
to be evicted AttemptToEvictConnection() since it'll have the worst
possible ping time and isn't providing blocks/transactions.
Therefore, we remove this check, and sent the recv buffer timeout to 20
minutes for all peers. This removes the final p2p version dependent
logic from the net layer, so all p2p version data can move into the
net_processing layer.
Alternative approaches:
- Set the recv buffer timeout to 90 minutes for all peers. This almost
wouldn't be a behaviour change at all (pre-BIP 31 peers would still
have the same recv buffer timeout, and we can't ever reach a recv buffer
timeout higher than 21 minutes for post-BIP31 peers, because the pong
timeout would be hit first).
- Stop supporting peers that don't support BIP 31. BIP 31 has been in
use since 2012, and implementing it is trivial.
|
|
|
|
It's not actually possible to change this value, so remove the
indirection of it being a conn option.
DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant.
|
|
This was introduced in 872fee3fccc8b33b9af0a401b5f85ac5504b57eb and it's unclear
if it's ever been used.
|
