28bac81a34 test: add functional test for getaddrmaninfo (stratospher)
c8eb8dae51 rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher)
Pull request description:
implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate.
This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.
```jsx
$ getaddrmaninfo
Result:
{ (json object) json object with network type as keys
"network" : { (json object) The network (ipv4, ipv6, onion, i2p, cjdns)
"new" : n, (numeric) number of addresses in new table
"tried" : n, (numeric) number of addresses in tried table
"total" : n (numeric) total number of addresses in both new/tried tables from a network
},
...
}
```
### additional context from [original PR](https://github.com/bitcoin/bitcoin/pull/26988)
1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137596851.
2. #26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808.
ACKs for top commit:
0xB10C:
ACK 28bac81a34
willcl-ark:
reACK 28bac81a34
achow101:
ACK 28bac81a34
brunoerg:
reACK 28bac81a34
theStack:
Code-review ACK 28bac81a34
Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a
ee589d4466 Add regression test for m_limit mutation (Greg Sanders)
275579d8c1 Remove MemPoolAccept::m_limits, only have local copies for carveouts (Greg Sanders)
Pull request description:
Without remoing it, if we ever call `PreChecks()` multiple times for any reason during any one `MempoolAccept`, subsequent invocations may have incorrect limits, allowing longer/larger chains than should be allowed.
Currently this is only an issue with `submitpackage`, so this is not exposed on mainnet.
ACKs for top commit:
achow101:
ACK ee589d4466
glozow:
ACK ee589d4466, nits can be ignored
ariard:
Code Review ACK ee589d446
Tree-SHA512: 14cf8edc73e014220def82563f5fb4192d1c2c111829712abf16340bfbfd9a85e2148d723af6fd4995d503dd67232b48dcf8b1711668d25b5aee5eab1bdb578c
fad52baf1e fuzz: Rework addr fuzzing (MarcoFalke)
fa5b6d29ee fuzz: Drop unused params from serialize helpers (MarcoFalke)
Pull request description:
Some minor fixups to addr fuzzing
ACKs for top commit:
dergoegge:
utACK fad52baf1e
Tree-SHA512: 6a2b07fb1a65cf855d5e7c0a52bfcb81d46dbc5d4b3e72cef359987cbd28dbfeb2fc54f210e9737cb131b40ac5f88a90e9af284e441e0b37196121590bbaf015
8e7e3e6149 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a2372a wallet: disallow migration of invalid or not-watched scripts (furszy)
Pull request description:
Fixing #28057.
The legacy wallet allows to import any raw script (#28126), without
checking if it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means `IsMineInner()` returning
`IsMineResult::INVALID` for them.
Note:
To verify this, can run the test commit on top of master.
`wallet_migration.py` will crash without the bugfix commit.
ACKs for top commit:
achow101:
ACK 8e7e3e6149
Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
ad0c469d98 wallet: Use CTxDestination in CRecipient rather than scriptPubKey (Andrew Chow)
07d3bdf4eb Add PubKeyDestination for P2PK scripts (Andrew Chow)
1a98a51c66 Allow CNoDestination to represent a raw script (Andrew Chow)
8dd067088d Make WitnessUnknown members private (Andrew Chow)
Pull request description:
For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address.
In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to `fundrawtransaction`), `CNoDestination` is changed to contain raw scripts.
Additionally, P2PK scripts are now interpreted as a new `PubKeyDestination` rather than `PKHash`. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct.
`ExtractDestination`'s behavior is slightly changed for the above. It now returns `true` for those destinations that have addresses, so P2PK scripts now result in `false`. Even though it returns false for `CNoDestination`, the script will now be included in that `CNoDestination`.
Builds on #28244
ACKs for top commit:
josibake:
ACK ad0c469d98
Tree-SHA512: ef3f8f3c7284779d9806c77c85b21caf910a79a1f7e7f1b51abcc0d7e074f14e00abf30f625a13075e41d94dad6202c10ddff462c0ee74c2ca4aab585b145a52
3f4e1bb9ae tests: fix incorrect assumption in v2transport_test (Pieter Wuille)
Pull request description:
One part of the current `v2transport_test` introduced in #28196 assumes that if a bit gets modified in a message, failure should instantly be detected after sending that message. This is not correct in case the length descriptor is modified, as that may cause the receiver to need more data first. Fix this by sending more messages until failure actually occurs.
Discovered in https://github.com/bitcoin/bitcoin/pull/27495#issuecomment-1719934041.
ACKs for top commit:
theStack:
ACK 3f4e1bb9ae
Tree-SHA512: faa90bf91996cbaaef62d764e746cb222eaf6796316b0d0e13709e528750b7c0ef09172f7fecfe814dbb8c136c5259f65ca1ac79318e6768a0bfc4e626a63249
508d05f8a7 [fuzz] Don't use afl++ deferred forkserver mode (dergoegge)
Pull request description:
Fixes #28469
This makes our afl++ harness essentially behave like libFuzzer, with the exception that the whole program does fully reset every 100000 iterations. 100000 is somewhat arbitrary and we could also go with `std::numeric_limits<unsigned in>::max()` but a smaller limit does allow for the occasional reset to counter act some amount of instability in the fuzzing loop (e.g. non-determinism, statefulness).
It's a bit of a shame to do this just for the targets whose initial state can't be forked (e.g. threads) because other targets do benefit from not having to redo the state setup. An alternative would be https://github.com/bitcoin/bitcoin/issues/28469#issuecomment-1717526774:
```
If the goal is to be maximally performant, the fork would need to happen for each fuzz target specifically.
I guess it can be achieved by wrapping __AFL_INIT(); into a helper function and then require all fuzz
target initialize() to call it?
```
ACKs for top commit:
MarcoFalke:
lgtm ACK 508d05f8a7
Tree-SHA512: d9fe94e2e3198795f8fb58f67eb383531a534bcd4ec75a1f0ae6ccb5531863dbc09800bb7d77536417745c4c8bc49a4f84dcc959918b27d4997a270eeacb0e7e
3fcd7fc7ff Do not use std::vector = {} to release memory (Pieter Wuille)
Pull request description:
It appears that invoking `v = {};` for an `std::vector<...> v` is equivalent to `v.clear()`, which does not release its allocated memory. There are a number of places in the codebase where it appears to be used for that purpose however (mostly written by me). Replace those with `std::vector<...>{}.swap(v);` (using a helper function `ClearShrink` in util/vector.h).
To explain what is going on: `v = {...};` is equivalent in general to `v.operator=({...});`. For many types, the `{}` is converted to the type of `v`, and then assigned to `v` - which for `std::vector` would ordinarily have the effect of clearing its memory (constructing a new empty vector, and then move-assigning it to `v`). However, since `std::vector<T>` has an `operator=(std::initializer_list<T>)` defined, it has precedence (since no implicit conversion is needed), and with an empty list, that is equivalent to `clear()`.
I did consider using `v = std::vector<T>{};` as replacement for `v = {};` instances where memory releasing is desired, but it appears that it does not actually work universally either. `V{}.swap(v);` does.
ACKs for top commit:
ajtowns:
utACK 3fcd7fc7ff
stickies-v:
ACK 3fcd7fc7ff
theStack:
Code-review ACK 3fcd7fc7ff
Tree-SHA512: 6148558126ec3c8cfd6daee167ec1c67b360cf1dff2cbc132bd71768337cf9bc4dda3e5a9cf7da4f7457d2123288eeba77dd78f3a17fa2cfd9c6758262950cc5
f18f9ef4d3 Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e944da Bump unconfirmed parent txs to target feerate (Murch)
3e3e052411 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889da66 [node] interface to get bump fees (glozow)
c24851be94 Make MiniMinerMempoolEntry fields private (Murch)
ac6030e4d8 Remove unused imports (Murch)
d2f90c31ef Fix calculation of ancestor set feerates in test (Murch)
a1f7d986e0 Match tx names to index in miniminer overlap test (Murch)
Pull request description:
Includes some commits to address follow-ups from #27021: https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156
Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.
While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.
Fixes #9645
Fixes #9864
Fixes #15553
ACKs for top commit:
S3RK:
ACK f18f9ef4d3
ismaelsadeeq:
ACK f18f9ef4d3
achow101:
ACK f18f9ef4d3
brunoerg:
crACK f18f9ef4d3
t-bast:
ACK f18f9ef4d3, I reviewed the latest changes and run e2e tests against eclair, everything looks good 👍
Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
de8f9123af test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b6 ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f24202 unit test: add coverage for BlockManager (Matthew Zipkin)
Pull request description:
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782
ACKs for top commit:
jonatack:
ACK de8f9123af modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
achow101:
ACK de8f9123af
MarcoFalke:
lgtm ACK de8f9123af📶
Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73
Deferring the forkserver initialization doesn't make sense for some of
our targets since they involve state that can't be forked (e.g.
threads). We therefore remove the use of __AFL_INIT entirely.
We also increase the __AFL_LOOP count to 100000. Our fuzz targets are
meant to all be deterministic and stateless therefore this should be
fine.
97e2e1d641 [fuzz] Use afl++ shared-memory fuzzing (dergoegge)
Pull request description:
Using shared-memory is faster than reading from stdin, see 7d2122e059/instrumentation/README.persistent_mode.md
ACKs for top commit:
MarcoFalke:
review ACK 97e2e1d641
Tree-SHA512: 7e71b5f84835e41531c19ee959be2426da245869757de8e5dd1c730ae83ead650e2ef75f4d594d7965f661821a4ffbd27be84d3ce623702991501b34a8d02fc3
d506765199 [refactor] Remove compat.h from kernel headers (TheCharlatan)
36193af47c [refactor] Remove netaddress.h from kernel headers (TheCharlatan)
2b08c55f01 [refactor] Add CChainParams member to CConnman (TheCharlatan)
f0d1d8b35c [refactor] Add missing includes for next commit (TheCharlatan)
534b314a74 kernel: Move MessageStartChars to its own file (TheCharlatan)
9be330b654 [refactor] Define MessageStartChars as std::array (TheCharlatan)
37e2b01113 [refactor] Allow std::array<std::byte, N> in serialize.h (MarcoFalke)
Pull request description:
This removes the non-consensus critical `protocol.h` and `netaddress.h` headers from the kernel headers. With this patch, they are no longer required to include in order to use the libbitcoinkernel library. This also allows for the removal of the `compat.h` header from the kernel headers.
As an added future benefit it also reduces the number of of kernel headers that include the platform specific `bitcoin-config.h`.
For those interested, the currently required kernel headers can be inspected visually with the [sourcetrail](https://github.com/CoatiSoftware/Sourcetrail) tool by looking at the required includes of `bitcoin-chainstate.cpp`.
---
This is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel.
ACKs for top commit:
stickies-v:
re-ACK d506765
hebasto:
ACK d506765199.
ajtowns:
utACK d506765199
MarcoFalke:
lgtm ACK d506765199🍛
Tree-SHA512: 6f90ea510a302c2927e84d16900e89997c39b8ff3ce9d4effeb8a134bd29cc52bd9e81e51aaa11f7496bad00025b78a58b88c5a9e0bb3f4ebbe9a76309215fb7
fa19c914f7 scripted-diff: Rename CBufferedFile to BufferedFile (MarcoFalke)
fa2f2413b8 Remove unused GetType() from CBufferedFile and CAutoFile (MarcoFalke)
5c2b3cd4b8 dbwrapper: Use DataStream for batch operations (TheCharlatan)
Pull request description:
This refactor is required for https://github.com/bitcoin/bitcoin/pull/28052 and https://github.com/bitcoin/bitcoin/pull/28451
Thus, split it out.
ACKs for top commit:
ajtowns:
utACK fa19c914f7
TheCharlatan:
ACK fa19c914f7
Tree-SHA512: d9c232324702512e45fd73ec3e3170f1e8a8c8f9c49cb613a6b693a9f83358914155527ace2517a2cd626a0cedcada56ef70a2a7812edafb1888fd6765eebba2
When a transaction uses an unconfirmed input, preceding this commit it
would not consider the feerate of the parent transaction. Given a parent
transaction with a lower ancestor feerate, this resulted in the new
transaction's ancestor feerate undershooting the target feerate.
This commit changes how we calculate the effective value of unconfirmed UTXOs.
The effective value of unconfirmed UTXOs is decreased by the fee
necessary to bump its ancestry to the target feerate. This also impacts
the calculation of the waste metric: since the estimate for the current
fee is increased by the bump fees, unconfirmed UTXOs current fees appear less
favorable compared to their unchanged long term fees.
This has one caveat: if multiple UTXOs have overlapping ancestries, each
of their individual estimates will account for bumping all ancestors.
GetSelectionWaste will need to access more context within a selection
result, and so should be a private member function rather than a static
function. It's only use outside of SelectionResult was for tests which
have now been updated to just make a SelectionResult.
Co-authored-by: Murch <murch@murch.one>
Follow-up from #27021: accessing of fields in MiniMinerMempoolEntry was
done inconsistently. Even though we had a getter, we would directly
write to the fields when we needed to update them.
This commits sets the fields to private and introduces a method for
updating the ancestor information in transactions using the same method
name as used for Mempool Entries.
Follow-up from #27021.
Also included is an ASCII art visualization of the test’s transaction
topology by theStack.
Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
Follow-up from #27021: In the prior commit, the vector started counting
at 0, but the transaction names started with 1. This commit matches the
names to the transactions’ vector indices for better readability.
Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
32c1dd1ad6 [test] mempool coins disappearing mid-package evaluation (glozow)
a67f460c3f [refactor] split setup in mempool_limit test (glozow)
d08696120e [test framework] add ability to spend only confirmed utxos (glozow)
3ea71feb11 [validation] don't LimitMempoolSize in any subpackage submissions (glozow)
d227b7234c [validation] return correct result when already-in-mempool tx gets evicted (glozow)
9698b81828 [refactor] back-fill results in AcceptPackage (glozow)
8ad7ad3392 [validation] make PackageMempoolAcceptResult members mutable (glozow)
03b87c11ca [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (glozow)
3f01a3dab1 [CCoinsViewMemPool] track non-base coins and allow Reset (glozow)
7d7f7a1189 [policy] check for duplicate txids in package (glozow)
Pull request description:
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in `m_view`, we don't realize the UTXO has disappeared until `CheckInputsFromMempoolAndCache` asserts that they exist. Also, the returned `PackageMempoolAcceptResult` reports that the transaction is in mempool even though it isn't anymore.
Fix this by not calling `LimitMempoolSize()` until the very end, and editing the results map with "mempool full" if things fall out.
Pointed out by instagibbs in faeed687e5 on top of the v3 PR.
ACKs for top commit:
instagibbs:
reACK 32c1dd1ad6
Tree-SHA512: 61e7f69db4712e5e5bfa27d037ab66bdd97f1bf60a8d9ffb96adb1f0609af012c810d681102ee5c7baec7b5fe8cb7c304a60c63ccc445d00d86a2b7f0e7ddb90
Don't do any mempool evictions until package validation is done,
preventing the mempool minimum feerate from changing. Whether we submit
transactions separately or as a package depends on whether they meet the
mempool minimum feerate threshold, so it's best that the value not
change while we are evaluating a package.
This avoids a situation where we have a CPFP package in which
the parents meet the mempool minimum feerate and are submitted by
themselves, but they are evicted before we have submitted the child.
Bug fix: a transaction may be in the mempool when package evaluation
begins (so it is added to results_final with MEMPOOL_ENTRY or
DIFFERENT_WITNESS), but get evicted due to another transaction
submission.
Instead of populating the last PackageMempoolAcceptResult with stuff
from results_final and individual_results_nonfinal, fill results_final
and create a PackageMempoolAcceptResult using that one.
A future commit will add LimitMempoolSize() which may change the status
of each of these transactions from "already in mempool" or "submitted to
mempool" to "no longer in mempool". We will change those transactions'
results here.
A future commit also gets rid of the last AcceptSubPackage outside of
the loop. It makes more sense to use results_final as the place where
all results end up.
(1) Call AcceptSingleTransaction when there is only 1 transaction in the
subpackage. This avoids calling PackageMempoolChecks() which enforces
rules that don't need to be applied for a single transaction, i.e.
disabling CPFP carve out.
There is a slight change in the error type returned, as shown in the
txpackage_tests change. When a transaction is the last one left in the
package and its fee is too low, this returns a PCKG_TX instead of
PCKG_POLICY. This interface is clearer; "package-fee-too-low" for 1
transaction would be a bit misleading.
(2) Clean up m_view and m_viewmempool so that coins created in this
sub-package evaluation are not available for other sub-package
evaluations. The contents of the mempool may change, so coins that are
available now might not be later.
Temporary coins should not be available in separate subpackage submissions.
Any mempool coins that are cached in m_view should be removed whenever
mempool contents change, as they may be spent or no longer exist.
Duplicates of normal transactions would be found by looking for
conflicting inputs, but this doesn't catch identical empty transactions.
These wouldn't be valid but exiting early is good and AcceptPackage's
result sanity checks assume non-duplicate transactions.
* Replace ConsumeDeserializationParams with V1, because V2 is
unconditionally checked as well.
* Also fuzz CAddress::Format::Disk in the address_deserialize fuzz
target.
This commit makes compat.h no longer a required include for users of the
libbitcoinkernel. Including compat.h imports a bunch of
platform-specific definitions.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Move functions requiring the netaddress.h include out of
libbitcoinkernel source files.
The netaddress.h file contains many non-consensus related definitions
and should thus not be part of the libbitcoinkernel. This commit makes
netaddress.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
The protocol.h file contains many non-consensus related definitions and
should thus not be part of the libbitcoinkernel. This commit makes
protocol.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
