668aa6af8d test: p2p: check that `getaddr` msgs are only responded once per connection (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing test coverage for ignoring repeated `getaddr` requests (introduced in #7856, commit 66b07247a7):
6f03c45f6b/src/net_processing.cpp (L4642-L4648)
ACKs for top commit:
MarcoFalke:
lgtm ACK 668aa6af8d
brunoerg:
crACK 668aa6af8d
Tree-SHA512: edcdc6501c684fb41911e393f55ded9b044cd2f92918877eca152edd5a4287d1a9d57ae999f1cb42185eae00c3a0af411fcb9bcd5b990ef48849c3834b141584
df60de770d log: Print error message when coindb is in inconsistent state (Fabian Jahr)
Pull request description:
While doing manual testing on assumeutxo this week I managed to put the coindb into an inconsistent state twice. For a normal user, this can also happen if their computer crashes during a flush or if they try to stop their node during a flush and then get tired of waiting and just shut their computer down or kill the process. It's an edge case but I wouldn't be surprised if this does happen more often when assumeutxo gets used more widely because there might be multiple flushes happening during loading of the UTXO set in the beginning and users may think something is going wrong because of the unexpected wait or they forgot some configs and want to start over quickly.
The problem is, when this happens at first the node starts up normally until it's time to flush again and then it hits an assert that the user can not understand.
```
2023-08-25T16:31:09Z [httpworker.0] [snapshot] 52000000 coins loaded (43.30%, 6768 MB)
2023-08-25T16:31:16Z [httpworker.0] Cache size (7272532192) exceeds total space (7256510300)
2023-08-25T16:31:16Z [httpworker.0] FlushSnapshotToDisk: flushing coins cache (7272 MB) started
Assertion failed: (old_heads[0] == hashBlock), function BatchWrite, file txdb.cpp, line 126.
Abort trap: 6
```
We should at least log an error message that gives users a hint of what the problem is and what they can do to resolve it. I am keeping this separate from the assumeutxo project since this issue can also happen during any regular flush.
ACKs for top commit:
jonatack:
ACK df60de770d
achow101:
ACK df60de770d
ryanofsky:
Code review ACK df60de770d
jamesob:
Code review ACK df60de770d
Tree-SHA512: b546aa0b0323ece2962867a29c38e014ac83ae8f1ded090da2894b4ff2450c05229629c7e8892f7b550cf7def4038a0b4119812e548e11b00c60b1dc3d4276d2
f2d4e510b3 ci: Avoid saving the same Ccache cache (Hennadii Stepanov)
14e5de6d02 ci: Avoid oversubscription in functional tests on Windows (Hennadii Stepanov)
Pull request description:
This PR aims to reduce the frequency of functional test failures on Windows like this [one](https://github.com/bitcoin/bitcoin/actions/runs/6040229997):
```
2023-09-01T01:05:01.850000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 552, in start_nodes
node.wait_for_rpc_connection()
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 296, in wait_for_rpc_connection
self._raise_assertion_error("Unable to connect to bitcoind after {}s".format(self.rpc_timeout))
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 177, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 1] Unable to connect to bitcoind after 2400s
```
This code has had zero failures in my personal repository in more than 25 runs (and is still counting).
---
The second commit is a minor improvement to avoid "Cache save failed." warnings during job re-runs. For [example](https://github.com/bitcoin/bitcoin/actions/runs/5998688759):
ACKs for top commit:
MarcoFalke:
lgtm ACK f2d4e510b3🐾
Tree-SHA512: 0c92817d37325a114886900e49a4d644201397d98d6ac9f2dcd41170c7e7ea2cb1873f7e51b5cb3ad3cc2e59554ad1c8f87d439ea6c1c960bf5c339153be7040
a3b55c94b9 [doc] move comment about AlreadyHaveTx DoS score to the right place (glozow)
3b8c17838a [log] add more logs related to orphan handling (glozow)
51b3275cd1 [log] add category TXPACKAGES for orphanage and package relay (glozow)
a33dde1e41 [log] include wtxid in tx {relay,validation,orphanage} logging (glozow)
Pull request description:
This was taken from #28031 (see #27463 for project tracking).
- Log wtxids in addition to txids when possible. This allows us to track the fate of a transaction from inv to mempool accept/reject through logs.
- Additional orphan-related logging to make testing and debugging easier. Suggested in https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1531022386 and https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1269622220
- Add `TXPACKAGES` category for logging.
- Move a nearby comment block that was in the wrong place.
ACKs for top commit:
instagibbs:
reACK a3b55c94b9
achow101:
ACK a3b55c94b9
brunoerg:
crACK a3b55c94b9
mzumsande:
Code review ACK a3b55c94b9
Tree-SHA512: 21884ef7c2ea2fd006e715574a9dd3e6cbbe8f82d62c6187fe1d39aad5a834051203fda5f355a06ca40c3e2b9561aec50d7c922a662b1edc96f7b552c9f4b24d
fa3b816240 doc: Fill in the required skills in the good_first_issue template (MarcoFalke)
Pull request description:
Compiling and running the tests is always required, so fill it in to avoid having to type it manually every time.
ACKs for top commit:
willcl-ark:
ACK fa3b816
Tree-SHA512: 1bcb93aaff235dd62513cda05547db90d12ad7638c050ee125845d20df1e1bc457bf4ec590677a0875fae8729dcc58842398e637e517997b35e3b3adffc34a72
32db15450a gui: make '-min' minimize wallet loading dialog (furszy)
Pull request description:
Simple fix for #748.
When '-min' is enabled, no loading dialog should
be presented on screen during startup.
ACKs for top commit:
hebasto:
ACK 32db15450a, tested on Debian 11 + XFCE.
Tree-SHA512: d08060b044938c67e8309db77b49ca645850fc21fdd7d78d5368d336fb9f602dcc66ea398a7505b00bf7d43afa07108347c7260480319fad3ec84cb41332f780
fa70cbd969 ci: Remove unused TEST_RUNNER_ENV="LC_ALL=C" from s390x task (MarcoFalke)
fa33354dcb ci: Remove /ro_base bind mount (MarcoFalke)
fa0df9d4c4 doc: Remove sudo from command that is already run as root (MarcoFalke)
Pull request description:
Remove some CI stuff no longer needed.
ACKs for top commit:
fanquake:
ACK fa70cbd969 - did not test the s390x job.
Tree-SHA512: 3a6ed0cfc855a92c2f834e59494c0a19a5647510247aece5e40a1aa78074894fe7454e684a1ea1f8f0662c50ac1caf2e390398b0fcfbf81544e6488fa9b8915e
faf7e69862 test: Support powerpc64le in get_previous_releases.py (MarcoFalke)
Pull request description:
To test: `test/get_previous_releases.py -b -t /tmp/prev_releases v22.0`
On master: `Not sure which binary to download for powerpc64le-unknown-linux-gnu`
Here: (pass)
ACKs for top commit:
fanquake:
ACK faf7e69862
Tree-SHA512: 33d9348f99e0d3924a6a5cba8833ec9e413e80167012b557922f3628069dabd555b02f98a6bfd0eb80e2bbbcdb50865b7bca216e1d080b1546ee4812abda4bc2
This comment isn't in the right place, as detection of a tx in
recent_rejects would cause the function to exit much earlier.
Move the comment to the right place and tweak the first sentence for
accuracy.
- Whenever a tx is erased. Allows somebody to see which transactions
have been erased due to expiry/overflow, not just how many.
- Whenever a tx is added to a peer's workset.
- AcceptToMemoryPool when a tx is accepted, mirroring the one logged for
a tx received from a peer. This allows someone to see all of the
transactions that are accepted to mempool just by looking for ATMP logs.
- MEMPOOLREJ when a tx is rejected, mirroring the one logged for
a tx received from a peer. This allows someone to see all of the
transaction rejections by looking at MEMPOOLREJ logs.
13eb8aa572 doc: Release notes for testnet defaulting to -acceptnonstdtxn=0 (Anthony Towns)
e1dc15d690 config: default acceptnonstdtxn=0 on all chains (Anthony Towns)
Pull request description:
Changes `-acceptnonstxtxn` to default to 0 on testnet, matching the other chains. Allowing non-standard txs on testnet by default contributed to the difficulties RSK described in #26348: "We see that there are two script paths and, to reduce the script size, a single CHECKMULTISIG is used for the two paths, separating the signer count from the CHECKMULTISIG opcode. This script worked on testnet, because it lacks the standard checks performed in Mainnet."
ACKs for top commit:
MarcoFalke:
lgtm ACK 13eb8aa572
sipa:
utACK 13eb8aa572
instagibbs:
utACK 13eb8aa572
theStack:
Code-review ACK 13eb8aa572
Tree-SHA512: eff7a3f9fc9b94003a730beb96e6f3399bc8b8e93fde4b15f20a11eda61d9a3e076f4423989f98b794b32681abecbc3756a54cd0d37b136e2fb2ffbb47ee7774
99995cfe8d refactor: Use HashWriter over legacy CHashWriter (via SerializeHash) (MarcoFalke)
5555aa2d0d refactor: Use HashWriter over legacy CHashWriter (MarcoFalke)
Pull request description:
`HashWriter` is a slim and less confusing version of `CHashWriter`, so use it in all places where it compiles.
This should be correct, if it compiles.
ACKs for top commit:
sipa:
That said, code review ACK 99995cfe8d
theuni:
ACK 99995cfe8d
TheCharlatan:
ACK 99995cfe8d
Tree-SHA512: fc967a18379bd00bd334ac3d50beb5435b65ca66a48f72623f1dcdbbce3292fd91839160cd0e69b8f4f3d98e258dcbbc6f73f5e91345f938898ee39c903a442b
Previously, the default for acceptnonstdtxn defaulted to 0 on all
chains except testnet. Change this to be consistent across all
chains, and remove the parameter from chainparams entirely.
ff42d81383 guix: use clang-toolchain-15 for macOS compilation (fanquake)
94955b4b1d depends: use LLVM/Clang 15.0.6 for macOS cross-compile (fanquake)
Pull request description:
This will end up being a blocker for #28210, and is already part of #21778, even though an even newer LLVM/Clang combination is required (and still missing from upstream Guix). Seems straight-forward enough to just bump the macOS compiler to a more modern Clang.
ACKs for top commit:
TheCharlatan:
re-ACK ff42d81383
Tree-SHA512: 8af4b54c3a56abb3825c6470444a28e14e9c69820c09ec4a33acebb8ae434df9ae18163c088a582130cc68755293a7e2bde5d065763919d94064ff9b3f83730f
b3a93b409e test: add functional test for deadlock situation (Martin Zumsande)
3557aa4d0a test: add basic tests for sendmsgtopeer to rpc_net.py (Martin Zumsande)
a9a1d69391 rpc: add test-only sendmsgtopeer rpc (Martin Zumsande)
Pull request description:
This adds a `sendmsgtopeer` rpc (for testing only) that allows a node to send a message (provided in hex) to a peer.
While we would usually use a `p2p` object instead of a node for this in the test framework, that isn't possible in situations where this message needs to trigger an actual interaction of multiple nodes.
Use this rpc to add test coverage for the bug fixed in #27981 (that just got merged):
The test lets two nodes (almost) simultaneously send a single large (4MB) p2p message to each other, which would have caused a deadlock previously (making this test fail), but succeeds now.
As can be seen from the discussion in #27981, it was not easy to reproduce this bug without `sendmsgtopeer`. I would imagine that `sendmsgtopeer` could also be helpful in various other test constellations.
ACKs for top commit:
ajtowns:
ACK b3a93b409e
sipa:
ACK b3a93b409e
achow101:
ACK b3a93b409e
Tree-SHA512: 6e22e72402f3c4dd70cddb9e96ea988444720f7a164031df159fbdd48056c8ac77ac53def045d9208a3ca07437c7c8e34f8b4ebc7066c0a84d81cd53f2f4fa5f
c8e066461b doc: Improve documentation of rpcallowip rpchelp (willcl-ark)
Pull request description:
Closes #21070
v21.0 introduced a behaviour changed noted in #21070 where using a config value `rpcallowip=::0` no longer also permitted ipv4 ip addresses.
The rpc_bind.py functional test covers this new behaviour already by checking that the list of bind addresses exactly matches what is expected so this commit only updates the documentation.
ACKs for top commit:
achow101:
ACK c8e066461b
pinheadmz:
ACK c8e066461b
jonatack:
ACK c8e066461b
Tree-SHA512: 332060cf0df0427c6637a9fd1e0783ce0b0940abdb41b0df13f03bfbdc28af067cec8f0b1bbc4e47b3d54fa1b2f110418442b05b39d5e7c7e0b96744ddd7c003
bf26f978ff fuzz: coinselection, fix `m_cost_of_change` (brunoerg)
6d9b26d56a fuzz: coinselection, BnB should never produce change (brunoerg)
b2eb558407 fuzz: coinselection, compare `GetSelectedValue` with target (brunoerg)
0df0438c60 fuzz: coinselection, improve `ComputeAndSetWaste` (brunoerg)
1e351e5db1 fuzz: coinselection, add coverage for `Merge` (brunoerg)
f0244a8614 fuzz: coinselection, add coverage for `GetShuffledInputVector`/`GetInputSet` (brunoerg)
808618b8a2 fuzz: coinselection, add coverage for `AddInputs` (brunoerg)
90c4e6a241 fuzz: coinselection, add coverage for `EligibleForSpending` (brunoerg)
2a031cb2c2 fuzz: coinselection, add `CreateCoins` (brunoerg)
Pull request description:
This PR:
- Moves coin creation to its own function called `CreateCoins`.
- Add coverage for `EligibleForSpending`
- Add coverage for `AddInputs`: get result of each algorithm (srd, knapsack and bnb), call `CreateCoins` and add into them.
- Add coverage for `GetShuffledInputVector` and `GetInputSet` using the result of each algorithm (srd, knapsack and bnb).
- Add coverage for `Merge`: Call SRD with the new utxos and, if successful, try to merge with the previous SRD result.
ACKs for top commit:
murchandamus:
reACK with some minimal fuzzing bf26f978ff
achow101:
ACK bf26f978ff
furszy:
re-ACK bf26f97
Tree-SHA512: bdd2b0a39de37be0a9b21a7c51260b6b8abe538cc0ea74312eb658b90a121a1ae07306c09fb0e75e93b531ce9ea2402feb041b0d852902d07739257f792e64ab
8a3b6f3387 refactor: make Transport::ReceivedBytes just return success/fail (Pieter Wuille)
bb4aab90fd net: move message conversion to wire bytes from PushMessage to SocketSendData (Pieter Wuille)
a1a1060fd6 net: measure send buffer fullness based on memory usage (Pieter Wuille)
009ff8d650 fuzz: add bidirectional fragmented transport test (Pieter Wuille)
fb2c5edb79 net: make V1Transport implicitly use current chainparams (Pieter Wuille)
0de48fe858 net: abstract sending side of transport serialization further (Pieter Wuille)
649a83c7f7 refactor: rename Transport class receive functions (Pieter Wuille)
27f9ba23ef net: add V1Transport lock protecting receive state (Pieter Wuille)
93594e42c3 refactor: merge transport serializer and deserializer into Transport class (Pieter Wuille)
Pull request description:
This PR furthers the P2P message serialization/deserialization abstraction introduced in #16202 and #16562, in preparation for introducing the BIP324 v2 transport (making this part of #27634). However, nothing in this PR is BIP324-specific, and it contains a number of independently useful improvements.
The overall idea is to have a single object in every `CNode` (called `m_transport`) that is responsible for converting sent messages to wire bytes, and for converting received wire bytes back to messages, while having as little as possible knowledge about this conversion process in higher-level net code. To accomplish that, there is an abstract `Transport` class with (currently) a single `V1Transport` implementation.
Structurally, the above is accomplished by:
* Merging the `TransportDeserializer` and `TransportSerializer` classes into a single `Transport` class, which encompasses both the sending and receiving side. For `V1Transport` these two sides are entirely separate, but this assumption doesn't hold for the BIP324 transport where e.g. the sending encryption key depends on the DH key negotiation data received from the other side. Merging the two means a future `V2Transport` can handle all this interaction without callers needing to be aware.
* Removing the assumption that each message is sent using a computed header followed by (unmodified) data bytes. To achieve that, the sending side of `Transport` mirrors what the receiver side does: callers can set a message to be sent, then ask what bytes must be sent out, and then allowing them to transition to the next message.
* Adding internal locks to protect the sending and receiving state of the `V1Transport` implementation. I believe these aren't strictly needed (opinions welcome) as there is no real way to use `Transport` objects in a multi-threaded fashion without some form of external synchronization (e.g. "get next bytes to send" isn't meaningful to call from multiple threads at the same time without mechanism to control the order they'll actually get sent). Still, I feel it's cleaner to make the object responsible for its own consistency (as we definitely do not want the entire object to be under a single external GUARDED_BY, as that'd prevent simultaneous sending and receiving).
* Moving the conversion of messages to bytes on the sending side from `PushMessage` to `SocketSendData`, which is needed to deal with the fact that a transport may not immediately be able to send messages.
This PR is not a refactor, though some commits are. Among the semantic changes are:
* Changing the send buffer pushback mechanism to trigger based on the memory usage of the buffer rather than the amount of bytes to be sent. This is both closer to the desired behavior, and makes the buffering independent from transport details (which is why it's included here).
* When optimistic send is not applicable, the V1 message checksum calculation now runs in the net thread rather than the message handling thread. I believe that's generally an improvement, as the message handling thread is far more computationally bottlenecked already.
* The checksum calculation now runs under the `CNode::cs_vSend` lock, which does mean no two checksum calculations for messages sent to the same node can run in parallel, even if running in separate threads. Despite that limitation, having the checksum for non-optimistic sends moved in the net thread is still an improvement, I believe.
* Statistics for per-message-type sent bytes are now updated when the bytes are actually handed to the OS rather than in `PushMessage`. This is because the actual serialized sizes aren't known until they've gone through the transport object.
A fuzz test of the entire `V1Transport` is included. More elaborate rationale for each of the changes can be found in the commit messages.
ACKs for top commit:
theStack:
re-ACK 8a3b6f3387
vasild:
ACK 8a3b6f3387
dergoegge:
Code review ACK 8a3b6f3387
Tree-SHA512: 26e9a6df47f1dd3e3f3edb4874edf365728e5a8bbc9d0d4d71fb6000cb2dfde5574902c47ffcf825af6743922f2ff9d31a5a38942a196f4ca6669122e15e42e4
