007d6f0e85 test: fix `AddNode` unit test failure on OpenBSD (Sebastian Falbesoner)
Pull request description:
On OpenBSD 7.4, the following check of the unit test `test_addnode_getaddednodeinfo_and_connection_detection` currently fails:
```
BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true}));
```
The reason for that is that this OS seemingly doesn't support the IPv4 shorthand notation with omitted zero-bytes:
```
$ ping 127.1
ping: no address associated with name
```
As a simple fix, this PR skips the check for this with a pre-processor #if. On NetBSD and FreeBSD, `127.1` is resolved correctly to localhost and hence the test passes (thanks to vasild for verifying on the latter!).
ACKs for top commit:
vasild:
ACK 007d6f0e85
Tree-SHA512: 8ab8393c490e1ecc140e8ff74f6fa4d26d0dd77e6a77a241cd198314b8c5afee7422f95351ca05f4c1742433dab77016a8ccb8d28062f8edd4b703a918a2bbda
`CBlockPolicyEstimator` will implement `CValidationInterface` and
subscribe to its notification to process transactions added and removed
from the mempool.
Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator.
Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`.
Co-authored-by: Matt Corallo <git@bluematt.me>
If alignment of the PoolAllocator would be insufficient, then the test would fail. This also catches the issue with ARM 32bit,
where int64_t is aligned to 8 bytes but void* is aligned to 4 bytes. The test adds a check to ensure the pool has allocated
a minimum number of chunks
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
This changes the PoolAllocator to default the alignment to the given type. This makes the code simpler, and most importantly
fixes a bug on ARM 32bit that caused OOM: The class CTxOut has a member CAmount which is an int64_t and on ARM 32bit int64_t
are 8 byte aligned which is larger than the pointer alignment of 4 bytes. So for CCoinsMap to be able to use the pool, we
need to use the alignment of the member instead of just alignof(void*).
This has the goal of prohibiting users from accidentally creating
runtime failures, e.g. by interacting with iterator_to with a copied
entry.
CTxMemPoolEntry is already implicitly not move-constructable. So be
explicit about this and use a std::list to collect the values in the
policy_estimator fuzz test instead of a std::vector.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
83986f464c Include version.h in fewer places (Anthony Towns)
c7b61fd61b Convert some CDataStream to DataStream (Anthony Towns)
1410d300df serialize: Drop useless version param from GetSerializeSize() (Anthony Towns)
bf574a7501 serialize: drop GetSerializeSizeMany (Anthony Towns)
efa9eb6d7c serialize: Drop nVersion from [C]SizeComputer (Anthony Towns)
Pull request description:
Drops the version field from `GetSerializeSize()`, simplifying the code in various places. Also drop `GetSerializeSizeMany()` (as just removing the version parameter could result in silent bugs) and remove unnecessary instances of `#include <version.h>`.
ACKs for top commit:
maflcko:
ACK 83986f464c📒
theuni:
ACK 83986f464c.
Tree-SHA512: 36617b6dfbb1b4b0afbf673e905525fc6d623d3f568d3f86e3b9d4f69820db97d099e83a88007bfff881f731ddca6755ebf1549e8d8a7762437dfadbf434c62e
faa25718b3 fuzz: AutoFile with XOR (MarcoFalke)
fab5cb9066 fuzz: Reduce LIMITED_WHILE limit for file fuzzing (MarcoFalke)
fa5388fad3 fuzz: Remove FuzzedAutoFileProvider (MarcoFalke)
Pull request description:
This should help to get fuzz coverage for https://maflcko.github.io/b-c-cov/fuzz.coverage/src/streams.cpp.gcov.html
Also, remove unused code and fix a timeout bug.
ACKs for top commit:
dergoegge:
ACK faa25718b3
Tree-SHA512: 56f1e6fd5cb2b66ffd9a7d9c09c9b8e396be3e7485feb03b35b6bd3c48e624fdaed50b472e4ffec21f09efb5e949d7ee32a13851849c9140b6b4cf25917dd7ac
43de4d3630 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
6a917918b7 fuzz: allow fake and duplicate inputs in tx_package_eval target (Greg Sanders)
a0626ccdad fuzz: allow reaching MempoolAcceptResult::ResultType::DIFFERENT_WITNESS in tx_package_eval target (Greg Sanders)
Pull request description:
Exercises `DIFFERENT_WITNESS` by using "blank" WSH() and allowing witness to determine wtxid, and attempts to make invalid/duplicate inputs.
ACKs for top commit:
dergoegge:
Coverage looks good to me ACK 6a917918b7
Tree-SHA512: db894f5f5b81c6b454874baf11f296462832285f41ccb09f23c0db92b9abc98f8ecacd72fc8f60dc92cb7947f543a2e55bed2fd210b0e8ca7c7d5389d90b14af
a0c254c13a Drop CHashWriter (Anthony Towns)
c94f7e5b1c Drop OverrideStream (Anthony Towns)
6e9e4e6130 Use ParamsWrapper for witness serialization (Anthony Towns)
Pull request description:
Choose whether witness is included in transaction serialization via serialization parameter rather than the stream version. See #25284 and #19477 for previous context.
ACKs for top commit:
maflcko:
re-ACK a0c254c13a🐜
theuni:
ACK a0c254c13a
Tree-SHA512: 8fd5cadfd84c5128e36c34a51fb94fdccd956280e7f65b7d73c512d6a9cdb53cdd3649de99ffab5322bd34be26cb95ab4eb05932b3b9de9c11d85743f50dcb13
1e5b86171e test: Add test for array serialization (TheCharlatan)
d49d198840 refactor: Initialize magic bytes in constructor initializer (TheCharlatan)
Pull request description:
This is a followup-PR for #28423
* Initialize magic bytes in constructor
* Add a small unit test for serializing arrays.
ACKs for top commit:
sipa:
utACK 1e5b86171e
maflcko:
lgtm ACK 1e5b86171e
Tree-SHA512: 0f58d2332dc501ca9fd419f40ed4f977c83dce0169e9a0eee1ffc9f8daa2d2ef7e7df18205ba076f55d90ae6c4a20d2b51ab303150d38470a962bcc58a66f6e7
3b70f7b615 doc: fix broken doc/design/multiprocess.md links after #24352 (Ryan Ofsky)
6d43aad742 span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3bdb9 util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c60f interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d682 interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754c00 interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327eaf3 interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379eca8 streams: Add SpanReader ignore method (Russell Yanofsky)
Pull request description:
This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR #10102, but have been moved here to make that PR smaller.
All of these changes are refactoring changes which do not affect behavior of current code
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 3b70f7b615
naumenkogs:
ACK 3b70f7b615
maflcko:
re-ACK 3b70f7b615🎆
Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c
fca0a8938e ci: remove "--exclude banman" for fuzzing in mac (brunoerg)
f9b286353f fuzz: call lookup functions before calling `Ban` (brunoerg)
Pull request description:
Fixes #27924
To not have any discrepancy, it's required to call lookup functions before calling `Ban`. If we don't do it, the assertion `assert(banmap == banmap_read);` may fail because `BanMapFromJson` will call `LookupSubNet` and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (#27924).
Also, calling lookup functions before banning is what RPC `setban` does.
ACKs for top commit:
maflcko:
lgtm ACK fca0a8938e
dergoegge:
ACK fca0a8938e
Tree-SHA512: a3d635088a556df4507e65542157f10b41d4f87dce42927b58c3b812f262f4544b6b57f3384eef1097ffdd7c32b8dd1556aae201254960cbfbf48d45551200f7
4dd94ca18f [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e804e [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939cab scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9cec [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c3b2 [refactor] remove access to mapTx in validation.cpp (glozow)
333367a940 [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855016 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdbf59 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e7a3 [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b744 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa561 [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8cafb77 [refactor] use exists() instead of mapTx.find() (glozow)
14804699e5 [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73abbd [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b4813eb [refactor] Add helper for iterating through mempool entries (stickies-v)
Pull request description:
Motivation
* It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
* Code external to mempool should ideally use its public helper methods instead of accessing `mapTx` or its iterators directly.
* Reduce the number of complex boost multi index type interactions
* Also see #28335 for further context/motivation. This PR together with #28385 simplifies that one.
Overview of things done in this PR:
* Make `vTxHashes` a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with `GetEntry` instead of being cached in `vTxHashes`.
* Introduce `GetEntry` helper method to replace the more involved `GetIter` where applicable
* Replace `mapTx` access with `CTxMemPool` helper methods
* Simplify `checkChainLimits` call in `node/interfaces.cpp`
* Make `CTxMemPoolEntry`s `lockPoints`mutable such that they can be changed with a const iterator directly instead of going through `mapTx`
* Make `BlockAssembler`'s `inBlock` and `failedTx` sets of transaction hashes.
ACKs for top commit:
glozow:
reACK 4dd94ca
maflcko:
re-ACK 4dd94ca18f👝
stickies-v:
re-ACK 4dd94ca18f
Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
fabb5046a7 fuzz: Avoid timeout and bloat in fuzz targets (MarcoFalke)
Pull request description:
If the fuzz input contains invalid data *in a loop*, abort early. This will teach the fuzz engine to look for useful data and avoids bloating the fuzz input folder with useless (repeated) data.
ACKs for top commit:
dergoegge:
utACK fabb5046a7
brunoerg:
crACK fabb5046a7
Tree-SHA512: 26da100d7558ae6fdd5292fb146d8858b2af8f78c546ca2509b9d27b33a33e9462ecb6035de142f9f36dd5de32f8cbad099d6c7a697902d23e1bb621cd27dc88
0420f99f42 Create net_peer_connection unit tests (Jon Atack)
4b834f6499 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443b net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d82 rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)
Pull request description:
## Rationale
Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.
### Connecting to the same node more than once
In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:
1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP
For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.
An example to test this would be calling:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" add
```
And check how it allows us to perform both connections some times, and some times it fails.
The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" onetry
```
### Adding the same peer with two different, yet equivalent, addresses
The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.
Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.
This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.
### Parametrize `CConnman::GetAddedNodeInfo`
Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`
ACKs for top commit:
jonatack:
re-ACK 0420f99f42
kashifs:
> > tACK [0420f9](0420f99f42)
sr-gi:
> > > tACK [0420f9](0420f99f42)
mzumsande:
Tested ACK 0420f99f42
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
1147e00e59 [validation] change package-fee-too-low, return wtxid(s) and effective feerate (glozow)
10dd9f2441 [test] use CheckPackageMempoolAcceptResult in previous tests (glozow)
3979f1afcb [validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN (glozow)
5c786a026a [refactor] use Wtxid for m_wtxids_fee_calculations (glozow)
Pull request description:
Split off from #26711 (suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253). This is part of #27463.
- Add 2 new TxValidationResults
- `TX_RECONSIDERABLE` helps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished from `TX_MEMPOOL_POLICY` so that we re-validate a transaction if and only if it is eligible for package CPFP. In the future, we will have a separate cache for reconsiderable rejects so these transactions don't go in `m_recent_rejects`.
- `TX_UNKNOWN` helps us communicate that we aborted package validation and didn't finish looking at this transaction: it's not valid but it's also not invalid (i.e. don't cache it as a rejected tx)
- Return effective feerate and the wtxids of transactions used to calculate that effective feerate when the error is `TX_SINGLE_FAILURE`. Previously, we would only provide this information if the transaction passed. Now that we have package validation, it's much more helpful to the caller to know how the failing feerate was calculated. This can also be used to improve our submitpackage RPC result (which is currently a bit unhelpful when things fail).
- Use the newly added `CheckPackageMempoolAcceptResult` for existing package validation tests. This increases test coverage and helps test the changes made in this PR.
ACKs for top commit:
instagibbs:
reACK 1147e00e59
achow101:
ACK 1147e00e59
murchandamus:
reACK 1147e00e59
ismaelsadeeq:
ACK 1147e00e59
Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
