MockableDatabase is a WalletDatabase that allows us to interact with the
records to change them independently from the wallet, as well as
changing the return values from within the tests. This will give us
greater flexibility in testing the wallet.
057057a2d7 Add test for `sendmany` rpc that uses `subtractfeefrom` parameter (Yusuf Sahin HAMZA)
Pull request description:
This PR adds test that uses `sendmany` rpc to send **BTC** to multiple addresses using `subtractfeefrom` parameter, then checks receiver addresses balances to make sure fees are subtracted correctly.
ACKs for top commit:
achow101:
ACK 057057a2d7
Tree-SHA512: 51167120d489f0ff7b8b9855424d07cb55a8965984f904643cddf45e7a08c350eaded498c350ec9c660edf72c2f128ec142347c9c79d5043d9f6cd481b15cd7e
9c18992bba test: add coverage for `-bantime` (brunoerg)
Pull request description:
This PR adds test coverage for `-bantime`. This flag sets the time in seconds how long the IP is banned (in the case you don't explicitly set `bantime` when using `setban`).
ACKs for top commit:
MarcoFalke:
lgtm ACK 9c18992bba
Tree-SHA512: e95f8608aa5df9b09cc5577daae662ed79ef5d5c69ee5e704d7c69520b9b51cc142e9e6be69d80356eda25a5215c4770b1a208638560c48cd3bc8f6d195a371f
b922f6b526 rpc: scanblocks, add "completed" flag to the result obj (furszy)
ce50acc54f rpc: scanblocks, do not traverse the whole chain block by block (furszy)
Pull request description:
Coming from https://github.com/bitcoin/bitcoin/pull/23549#pullrequestreview-1105712566
The current `scanblocks` flow walks-through every block in the active chain
until hits the chain tip or processes 10k blocks, then calls `lookupFilterRange`
function to obtain all filters from that particular range.
This is only done to obtain the heights range to look up the block
filters. Which is unneeded.
As `scanblocks` only lookup block filters in the active chain, we can
directly calculate the lookup range heights, by using the chain tip,
without requiring to traverse the chain block by block.
ACKs for top commit:
achow101:
ACK b922f6b526
TheCharlatan:
ACK b922f6b526
Tree-SHA512: 0587e6d9cf87a59184adb2dbc26a4e2bce3a16233594c6c330f69feb49bf7dc63fdacf44fc20308e93441159ebc604c63eb7de19204d3e745a2ff16004892b45
be72663a15 test: bumpfee, add coverage for "send coins back to yourself" (furszy)
7bffec6715 bumpfee: enable send coins back to yourself (furszy)
Pull request description:
Simple example:
1) User_1 sends 0.1 btc to user_2 on a low fee transaction.
2) After few hours, the tx is still in the mempool, user_2
is not interested anymore, so user_1 decides to cancel
it by sending coins back to himself.
3) User_1 has the bright idea of opening the explorer and
copy the change output address of the transaction. Then
call bumpfee providing such output (in the "outputs" arg).
Currently, this is not possible. The wallet fails with
"Unable to create transaction. Transaction must have at least
one recipient" error.
The error reason is because we discard the provided output
from the recipients list and set it inside the coin control
so the process adds it later (when the change is calculated).
But.. there is no later if the tx has no outputs.
ACKs for top commit:
ishaanam:
reACK be72663a15
achow101:
ACK be72663a15
Tree-SHA512: c2c38290a998f9b426a830d9624c7feb730158980ac186f8fb0138d5e200935d6538307bc60a2c3d0b7b6ee2b4ffb77a1e98baf8feb1d20a7d825f6055ac377f
9141e4395a rpc, docs: Add note for commands that supports only legacy wallets (Yusuf Sahin HAMZA)
Pull request description:
Refs #25363, apparently issue is not updated since over a month, so i decided to put the same `importaddress` note in #25368 to other rpc commands that needs this note.
Note is added for following commands:
- `importprivkey`
- `importpubkey`
- `importwallet`
- `dumpprivkey`
- `dumpwallet`
- `importmulti`
- `addmultisigaddress`
- `sethdseed`
ACKs for top commit:
achow101:
ACK 9141e4395a
Tree-SHA512: f3dc05d26577fd8dbe2bd69cb5c14ffccebacd6010402af44427b3d01be8484895dfcf33d55dfa766eadb7f9f3bae5cc4c2add3ac816a2ac60e8beb5a97527f3
a5986e82dd refactor: Remove CAddressBookData::destdata (Ryan Ofsky)
5938ad0bdb wallet: Add DatabaseBatch::ErasePrefix method (Ryan Ofsky)
Pull request description:
This is cleanup that doesn't change external behavior. Benefits of the cleanup are:
- Removes awkward `StringMap` intermediate representation for wallet address metadata.
- Simplifies `CWallet`, deals with used address and received request serialization in `walletdb.cpp` instead of higher level wallet code
- Adds test coverage and documentation
This PR doesn't change externally observable behavior. Internally, the only change in behavior is that `EraseDestData` deletes rows directly from the database because they are no longer stored in memory. This is more direct and efficient because it uses a single lookup and scan instead of multiple lookups.
Motivation for this cleanup is making changes like #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs
---
This PR is a rebased copy of #18608. For some reason that PR is locked and couldn't be reopened or commented on.
This PR is an alternative to #27215 with differences described in https://github.com/bitcoin/bitcoin/pull/27215#pullrequestreview-1329028143
ACKs for top commit:
achow101:
ACK a5986e82dd
furszy:
Code ACK a5986e82
Tree-SHA512: 6bd3e402f1f60263fbd433760bdc29d04175ddaf8307207c4a67d59f6cffa732e176ba57886e02926f7a1615dce0ed9cda36c8cbc6430aa8e5b56934c23f3fe7
To tell the user whether the process was aborted or not.
Plus, as the process can be aborted prior to the end range,
have also changed the "to_height" result value to return the
last scanned block instead of the end range block.
The current flow walks-through every block in the active chain until
hits the chain tip or processes 10k blocks, then calls
`lookupFilterRange()` to obtain all the filters from that
particular range.
This is only done to obtain the heights range to look up the block
filters. Which is unneeded.
As `scanblocks` only lookup block filters in the active chain, we can
directly calculate the lookup range heights, by using the chain tip,
without requiring to traverse the chain block by block.
fac395e5eb ci: Bump ci/lint/Dockerfile (MarcoFalke)
fa6eb65167 test: Use python3.8 pow() (MarcoFalke)
88881cf7ac Bump python minimum version to 3.8 (MarcoFalke)
Pull request description:
There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
* There is no supported operating system that ships 3.7 by default. (debian:buster is EOL and unmaintained to the extent that it doesn't run in the CI environment. See https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
* Compiling python 3.7 from source is also unsupported on at least macos, according to https://github.com/bitcoin/bitcoin/pull/24017#issuecomment-1107820790
* Recent versions of lief require 3.8, see https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1517561645
Fix all maintenance issues by bumping the minimum.
ACKs for top commit:
RandyMcMillan:
ACK fac395e
fjahr:
ACK fac395e5eb
fanquake:
ACK fac395e5eb
Tree-SHA512: c198decdbbe29d186d73ea3f6549d8a38479383495d14a965a2f9211ce39637b43f13a4c2a5d3bf56e2d468be4bbe49b4ee8e8e19ec69936ff43ddf2b714c712
33fdfc7986 test: perturb anchors.dat to test it doesn't throw an error during initialization (brunoerg)
Pull request description:
Got some inspiration from `feature_init`. This PR tests whether perturbing `anchors.dat` doesn't throw any error during initialization.
3f1f5f6f1e/src/addrdb.cpp (L223-L235)
ACKs for top commit:
MarcoFalke:
lgtm ACK 33fdfc7986
Tree-SHA512: e6584debb37647677581fda08366b45b42803022cc4c4f1d5a7bd5e9e04d64da77656dad2b804855337487bdcfc891f300a2e03668d6122de769dd14f39af9ed
4bdcf57158 test: test banlist database recreation (brunoerg)
Pull request description:
This PR adds test coverage for 'banlist database recreation'. If it wasn't able to read ban db (in `LoadBanlist`), so it should create a new (an empty, ofc) one.
d8bdee0fc8/src/banman.cpp (L28-L45)
ACKs for top commit:
MarcoFalke:
lgtm ACK 4bdcf57158
Tree-SHA512: d9d0cd0c4b3797189dff00d3a634878188e7cf51e78346601fc97e2bf78c495561705214062bb42ab8e491e0d111f8bfcf74dbc801768bc02cf2b45f162aad85
cca4f82b82 test: add coverage for rpc error when trying to rescan beyond pruned data (brunoerg)
Pull request description:
This PR adds test coverage for the following rpc error:
15692e2641/src/wallet/rpc/transactions.cpp (L896-L899)
ACKs for top commit:
MarcoFalke:
lgtm ACK cca4f82b82
aureleoules:
ACK cca4f82b82
Tree-SHA512: 724a055e9f6cddf1935699e8769015115f24f6485a0bd87e8660072ee44a15c1bddfdda848acc101ea7184b7e65a33b5b0d80b563d2ba3ecdab7a631378d6476
0c520679ab doc: add release notes for `abandoned` field in `gettransaction` and `listtransactions` (brunoerg)
a1aaa7f51f rpc, wallet: add `abandoned` field for all categories of transactions in ListTransactions (brunoerg)
Pull request description:
Fixes#25130
ACKs for top commit:
achow101:
re-ACK 0c520679ab
Tree-SHA512: 1864460d76decab7898737c96517d722055eb8f81ca52248fe1035723258c6cd4a93251e06a86ecbbb0b0a80af1466b2c86fb142ace4ccb74cc40d5dc3967d7f
bf77fc9cb4 [test] mempool full in package accept (glozow)
b51ebccc28 [validation] set PackageValidationState when mempool full (glozow)
563a2ee4f5 [policy] disallow transactions under min relay fee, even in packages (glozow)
c4554fe894 [test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee (glozow)
ac463e87df [test util] mock mempool minimum feerate (glozow)
Pull request description:
Part of package relay, see #27463.
Note that this still allows packages to bump transactions that are below the dynamic mempool minimum feerate, which means this still solves the "mempool is congested and my presigned 1sat/vB tx is screwed" problem for all transactions.
On master, the package policy (only accessible through regtest-only RPC submitpackage) allows 0-fee (or otherwise below min relay feerate) transactions if they are bumped by a child. However, with default package limits, we don't yet have a DoS-resistant way of ensuring these transactions remain bumped throughout their time in the mempool. Primarily, the fee-bumping child may later be replaced by another transaction that doesn't bump the parent(s). The parent(s) could potentially stay bumped by other transactions, but not enough to ever be selected by the `BlockAssembler` (due to `blockmintxfee`).
For example, (tested [here](https://github.com/glozow/bitcoin/commits/26933-motivation)):
- The mempool accepts 24 below-minrelayfeerate transactions ("0-fee parents"), all bumped by a single high-fee transaction ("the fee-bumping child"). The fee-bumping child also spends a confirmed UTXO.
- Two additional children are added to each 0-fee parent. These children each pay a feerate slightly above the minimum relay feerate (e.g. 1.9sat/vB) such that, for each 0-fee parent, the total fees of its two children divided by the total size of the children and parent is above the minimum relay feerate.
- If a block template is built now, all transactions would be selected.
- A transaction replaces the the fee-bumping child, spending only the confirmed UTXO and not any of the outputs from the 0-fee parents.
- The 0-fee parents now each have 2 children. Their descendant feerates are above minrelayfeerate, which means that they remain in the mempool, even if the mempool evicts all below-minrelayfeerate packages.
- If a block template is built now, none of the 0-fee parents or their children would be selected.
- Even more low-feerate descendants can be added to these below-minrelayfeerate packages and they will not be evicted until they expire or the mempool reaches capacity.
Unless we have a DoS-resistant way of ensuring package CPFP-bumped transactions are always bumped, allowing package CPFP to bump below-minrelayfeerate transactions can result in these problematic situations. See #27018 which proposes a partial solution with some limitations, and contains discussion about potential improvements to eviction strategy. While no adequate solution exists, for now, avoid these situations by requiring all transactions to meet min relay feerate.
ACKs for top commit:
ajtowns:
reACK bf77fc9cb4
instagibbs:
re-ACK bf77fc9cb4
Tree-SHA512: 28940f41493a9e280b010284316fb8caf1ed7b2090ba9a4ef8a3b2eafc5933601074b142f4f7d4e3c6c4cce99d3146f5c8e1393d9406c6f2070dd41c817985c9
96bf0bca4a test: simplify uint256 (de)serialization routines (Sebastian Falbesoner)
Pull request description:
These routines look fancy, but do nothing more than converting between byte objects of length 32 to/from integers in little endian byte order and can be replaced by simple one-liners, using the `int.{from,to}_bytes` methods (available since Python 3.2).
ACKs for top commit:
MarcoFalke:
lgtm ACK 96bf0bca4a
brunoerg:
crACK 96bf0bca4a
Tree-SHA512: f3031502d61a936147867ad8a0efa841a9bbdd2acf8781653036889a38524f4f1a5c86b1e07157bf2d9663097e7b84be6846678d0883d2a334beafd87e9510f0
These routines look fancy, but do nothing more than converting between
byte objects of length 32 to/from integers in little endian byte order
and can be replaced by simple one-liners, using the int.{from,to}_bytes
methods (available since Python 3.2).
10a354f174 test: prevent intermittent failures (Amiti Uttarwar)
Pull request description:
Follow up to #27214 - add an address to the tried table before the new table to make sure a new table collision is not possible.
ACKs for top commit:
mzumsande:
Code review ACK 10a354f174 - the fix is what I suggested [here](https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1169169601) and should make these intermittent failures impossible.
Tree-SHA512: 24099f02e1915395130065af0ef6a2a1893955d222517d156d928765541d9c427da00172a9b5a540163f4d6aae93ca3882e8267eeb35ecc595d42178abc6191c
6e9f8bb050 rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex (ishaanam)
a5b4883fb4 rpc: extract psbt updating logic into ProcessPSBT (ishaanam)
Pull request description:
Previously the `utxoupdatepsbt` RPC, added in #13932, only updated the inputs spending segwit outputs with the `witness_utxo`, because the full transaction is needed for legacy inputs. Before this RPC looked for just the utxos in the utxo set and the mempool. This PR makes it so that if the user has txindex enabled, then the full transaction is looked for there for all inputs. If it is not found in the txindex or txindex isn't enabled, then the mempool is checked for the full transaction. If the transaction an input is spending from is still not found at that point, then the utxo set is searched and the inputs spending segwit outputs are updated with just the utxo.
ACKs for top commit:
achow101:
ACK 6e9f8bb050
Xekyo:
ACK 6e9f8bb050
Tree-SHA512: 078db3c37a1ecd5816d80a42e8bd1341e416d661f508fa5fce0f4e1249fefd7b92a0d45f44957781cb69d0953145ef096ecdd4545ada39062be27742402dac6f
63c0c4ff10 depends: reuse _config_opts for CMake options (Cory Fields)
Pull request description:
Context: I'm [currently experimenting with building our depends with CMake when possible](https://github.com/theuni/bitcoin/commits/depends-cmake) as part of our future transition to CMake. Specifically zmq, libevent, libnatpmp, and miniupnpc all have existing CMake buildsystems. Building them with CMake will allow us to drop several deps that we currently have (autoconf, automake, pkg-config, etc) which would be unfortunate to carry over after the switch-over.
But that's not relevant for this PR. This is just a very simple change that makes the above work easier to experiment with as it [adds a needed feature for CMake packages](5733dc2000 (diff-e6ed342a25092e0a6d0308e0bfd826044578847132cc6726ac4afa2ca767b61aR20)). It's a no-op for the current builds.
---
From commit description:
This will allow us to use the existing machinery for filtering by arch, os, debug/release, etc.
For example, the following becomes possible for libevent when building with CMake:
`$(package)_config_opts_release=-DEVENT__DISABLE_DEBUG_MODE`
Now the define is only set when not building depends with DEBUG=1
ACKs for top commit:
hebasto:
ACK 63c0c4ff10
Tree-SHA512: 17d123219d2f98c017880196966ffebd5d09d3e2db8e443e227eb7e49da46e9d971fbe7fd2b99a324fc367e1bbe0ac3cd15b399bce98dd87fbb144d767e15fe7
faa7144d3c fuzz: re-enable prioritisetransaction & analyzepsbt RPC (MarcoFalke)
Pull request description:
The linked issue seems fixed, so it should be fine to re-enable
ACKs for top commit:
dergoegge:
utACK faa7144d3c
Tree-SHA512: a681c726fceacc27ab5a03d455c7808d33f3cb11fe7d253d455526568af840b29f0c3c1d97c54785ef9277e7891a3aa742ac73ccd3cf115b7606eba50864aaa9
be55f545d5 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d5🚲
ryanofsky:
Code review ACK be55f545d5. Just small cleanups since the last review.
hebasto:
ACK be55f545d5, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
* The GCC suppression was fixed in gcc-11, which is available on all LTS
releases of Linux distros.
* The feerate suppression was likely fixed and does not trigger anymore.
If it was to trigger again, the underlying bug should be fixed instead
of suppressing it.
* The bench suppression does not trigger anymore.
Also, add comments to tsan suppressions on how to reproduce.
0076bed45e logging: log ASN when using `-asmap` (brunoerg)
9836c76ae0 net: add `GetMappedAS` in `CConnman` (brunoerg)
Pull request description:
When using `-asmap`, you can check the ASN assigned to the peers only with the RPC command `getpeerinfo` (check `mapped_as` field), however, it's not possible to check it in logs (e.g. see in logs the ASN of the peers when a new outbound peer has been connected). This PR includes the peers' ASN in debug output when using `-asmap`.
Obs: Open this primarily to chase some Concept ACK, I've been using this on my node to facilitate to track the peers' ASN especially when reading the logs.
ACKs for top commit:
Sjors:
tACK 0076bed45e
jamesob:
ACK 0076bed45e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`](https://github.com/jamesob/bitcoin/tree/ackr/27412.1.brunoerg.logging_net_add_asn_from))
achow101:
ACK 0076bed45e
Tree-SHA512: c19cd11e8ab49962021f390459aadf6d33d221ae9a2c3df331a25d6865a8df470e2c8828f6e5219b8a887d6ab5b3450d34be9e26c00cca4d223b4ca64d51111b
25ab14712b refactor: coinselector_tests, unify wallet creation code (furszy)
ba9431c505 test: coverage for bnb max weight (furszy)
5a2bc45ee0 wallet: clean post coin selection max weight filter (furszy)
2d112584e3 coin selection: BnB, don't return selection if exceeds max allowed tx weight (furszy)
d3a1c098e4 test: coin selection, add coverage for SRD (furszy)
9d9689e5a6 coin selection: heap-ify SRD, don't return selection if exceeds max tx weight (furszy)
6107ec2229 coin selection: knapsack, select closest UTXO above target if result exceeds max tx size (furszy)
1284223691 wallet: refactor coin selection algos to return util::Result (furszy)
Pull request description:
Coming from the following comment https://github.com/bitcoin/bitcoin/pull/25729#discussion_r1029324367.
The reason why we are adding hundreds of UTXO from different sources when the target
amount is covered only by one of them is because only SRD returns a usable result.
Context:
In the test, we create 1515 UTXOs with 0.033 BTC each, and 1 UTXO with 50 BTC. Then
perform Coin Selection to fund 49.5 BTC.
As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the
expectation here is to receive a selection result that only contain the big UTXO.
Which is not happening for the following reason:
Knapsack returns a result that exceeds the max allowed transaction size, when
it should return the closest utxo above the target, so we fallback to SRD who
selects coins randomly up until the target is met. So we end up with a selection
result with lot more coins than what is needed.
ACKs for top commit:
S3RK:
ACK 25ab14712b
achow101:
ACK 25ab14712b
Xekyo:
reACK 25ab14712b
theStack:
Code-review ACK 25ab14712b
Tree-SHA512: 2425de4cc479b4db999b3b2e02eb522a2130a06379cca0418672a51c4076971a1d427191173820db76a0f85a8edfff100114e1c38fb3b5dc51598d07cabe1a60
9f947fc3d4 Use PoolAllocator for CCoinsMap (Martin Leitner-Ankerl)
5e4ac5abf5 Call ReallocateCache() on each Flush() (Martin Leitner-Ankerl)
1afca6b663 Add PoolResource fuzzer (Martin Leitner-Ankerl)
e19943f049 Calculate memory usage correctly for unordered_maps that use PoolAllocator (Martin Leitner-Ankerl)
b8401c3281 Add pool based memory resource & allocator (Martin Leitner-Ankerl)
Pull request description:
A memory resource similar to `std::pmr::unsynchronized_pool_resource`, but optimized for node-based containers. The goal is to be able to cache more coins with the same memory usage, and allocate/deallocate faster.
This is a reimplementation of #22702. The goal was to implement it in a way that is simpler to review & test
* There is now a generic `PoolResource` for allocating/deallocating memory. This has practically the same API as `std::pmr::memory_resource`. (Unfortunately I cannot use std::pmr because libc++ simply doesn't implement that API).
* Thanks to sipa there is now a fuzzer for PoolResource! On a fast machine I ran it for ~770 million executions without finding any issue.
* The estimation of the correct node size is now gone, PoolResource now has multiple pools and just needs to be created large enough to have space for the unordered_map nodes.
I ran benchmarks with #22702, mergebase, and this PR. Frequency locked Intel i7-8700, clang++ 13.0.1 to reindex up to block 690000.
```sh
bitcoind -dbcache=5000 -assumevalid=00000000000000000002a23d6df20eecec15b21d32c75833cce28f113de888b7 -reindex-chainstate -printtoconsole=0 -stopatheight=690000
```
The performance is practically identical with #22702, just 0.4% slower. It's ~21% faster than master:
Note that on cache drops mergebase's memory doesnt go so far down because it does not free the `CCoinsMap` bucket array.
ACKs for top commit:
LarryRuane:
ACK 9f947fc3d4
achow101:
re-ACK 9f947fc3d4
john-moffett:
ACK 9f947fc3d4
jonatack:
re-ACK 9f947fc3d4
Tree-SHA512: 48caf57d1775875a612b54388ef64c53952cd48741cacfe20d89049f2fb35301b5c28e69264b7d659a3ca33d4c714d47bafad6fd547c4075f08b45acc87c0f45
17e705428d doc: clarify new_only param for Select function (Amiti Uttarwar)
b0010c83a1 bench: test select for a new table with only one address (Amiti Uttarwar)
9b91aae085 bench: add coverage for addrman select with network parameter (Amiti Uttarwar)
22a4d1489c test: increase coverage of addrman select (without network) (Amiti Uttarwar)
a98e542e0c test: add addrman test for special case (Amiti Uttarwar)
5c8b4baff2 tests: add addrman_select_by_network test (Amiti Uttarwar)
6b229284fd addrman: add functionality to select by network (Amiti Uttarwar)
26c3bf11e2 scripted-diff: rename local variables to match modern conventions (Amiti Uttarwar)
48806412e2 refactor: consolidate select logic for new and tried tables (Amiti Uttarwar)
ca2a9c5f8f refactor: generalize select logic (Amiti Uttarwar)
052fbcd5a7 addrman: Introduce helper to generalize looking up an addrman entry (Amiti Uttarwar)
9bf078f66c refactor: update Select_ function (Amiti Uttarwar)
Pull request description:
For the full context & motivation of this patch, see #27213
This is joint work with mzumsande.
This PR adds functionality to `AddrMan::Select` to enable callers to specify a network they are interested in.
Along the way, it refactors the function to deduplicate the logic, updates the local variables to match modern conventions, adds test coverage for both the new and existing `Select` logic, and adds bench tests for the worst case performance of both the new and existing `Select` logic.
This functionality is used in the parent PR.
ACKs for top commit:
vasild:
ACK 17e705428d
brunoerg:
re-ACK 17e705428d
ajtowns:
ACK 17e705428d
mzumsande:
Code Review ACK 17e705428d
Tree-SHA512: e99d1ce0c44a15601a3daa37deeadfc9d26208a92969ecffbea358d57ca951102d759734ccf77eacd38db368da0bf5b6fede3cd900d8a77b3061f4adc54e52d8
9c24826e7b doc: Remove 25.0 release note fragments (Andrew Chow)
088a93dce8 build: Bump to 25.99 (Andrew Chow)
Pull request description:
Pre-25.x branch off version bump and release note fragments removal.
The 25.0 draft release notes are in the dev wiki: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Notes-Draft
ACKs for top commit:
fanquake:
ACK 9c24826e7b
Tree-SHA512: f7c7b04aa904e946bc672b5b07082a819b9d76ebccda0838bc27d0e6179cfb88b8f110500d5ea815f711580916bcfa0275774ec50a7298a4c66e645647111125
31b1798d2c p2p: update hardcoded mainnet seeds for 25.x (Jon Atack)
04dd1d3926 contrib: make-seeds updates for 25.x (Jon Atack)
f5c8788628 p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x (Jon Atack)
Pull request description:
Update the hardcoded P2P network seeds for 25.x after updating the manual seeds and the generation script as necessary. Previous update was #25911.
The manual seeds are selected for reachability, uptime and service bit 1 and/or curated trusted peers. We need more Tor and CJDNS seeds and some of the current Tor and I2P seeds are no longer reachable.
Can be tested by following the steps in `contrib/seeds/README.md` and verifying the manual seeds by checking their presence and services in getnodeaddresses and/or connecting to them and checking their services with getpeerinfo and behavior with -netinfo.
Tool output:
```
$ python3 makeseeds.py -a asmap-filled.dat -s seeds_main.txt > nodes_main.txt
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
IPv4 IPv6 Onion Pass
3972 1118 0 Initial
3972 1118 0 Skip entries with invalid address
3972 1118 0 After removing duplicates
3946 1112 0 Enforce minimal number of blocks
3946 1112 0 Require service bit 1
2791 798 0 Require minimum uptime
2757 788 0 Require a known and recent user agent
2757 788 0 Filter out hosts with multiple bitcoin ports
512 289 0 Look up ASNs and limit results per ASN and per net```
ACKs for top commit:
achow101:
ACK 31b1798d2c
mzumsande:
reACK 31b1798d2c
Tree-SHA512: 40cdd0ac74e3d26975ab688ee4af09bedcf15f6e02311757b27c91aafdc4da16cca2be90fee767207bb4ccd542ad19197e4d5e00011185018239de35da19c174
a2bef805c1 kernel: update m_assumed_* chain params for 25.x (fanquake)
4128e01dba kernel: update chainTxData for 25.x (fanquake)
00b2b114b4 kernel: update nMinimumChainWork & defaultAssumeValid for 25.x (fanquake)
07fcc0a82c doc: update references to kernel/chainparams.cpp (fanquake)
Pull request description:
Update chainparams pre `25.x` branch off.
Co-Author in the commits as a PR (#27223) had previously been opened too-early to do the same.
Note: Remember that some variance is expected in the `m_assumed_*` sizes.
ACKs for top commit:
achow101:
ACK a2bef805c1
josibake:
ACK a2bef805c1
gruve-p:
ACK a2bef805c1
dergoegge:
ACK a2bef805c1 on the new mainnet params
Tree-SHA512: 0b19c2ef15c6b15863d6a560a1053ee223057c7bfb617ffd3400b1734cee8f75bc6fd7f04d8f8e3f5af6220659a1987951a1b36945d6fe17d06972004fd62610
3cc989da5c Fix checking bad dns seeds without casting (Yusuf Sahin HAMZA)
Pull request description:
- Since seed lines comes with `str` type, comparing `good` column directly with **0** (`int` type) in the if statement was not working at all. This is fixed by casting `int` type to the values in the `good` column of seeds text file.
- Lines that starts with comment in the seeds text file are now ignored.
- If statement for checking bad seeds are moved to the top of the `parseline` function as if a seed is bad; there is no point of going forward from there.
Since this bug-fix eliminates bad seeds over **550k** in the first place, in my case; particular job for parsing all seeds speed is up by **600%** and whole script's speed is up by **%30**.
Note that **stats** in the terminal are not going to include bad seeds after this fix, which would be the same if this bug were never there before.
ACKs for top commit:
achow101:
ACK 3cc989da5c
jonatack:
ACK 3cc989da5c
Tree-SHA512: 13c82681de4d72de07293f0b7f09721ad8514a2ad99b0584d1c94fa5f2818821df2000944f9514d6a222a5dccc82856d16c8c05aa36d905cfa7d4610c629fd38
This will allow us to use the existing machinery for filtering by
arch, os, debug/release, etc.
For example, the following becomes possible for libevent:
$(package)_config_opts_release=-DEVENT__DISABLE_DEBUG_MODE
Now the define is only set when not building depends with DEBUG=1
4de9c2a65f ci: build libc++ with assertions in MSAN jobs (fanquake)
23b8b2026a ci: build libc++ in DEBUG mode in MSAN jobs (fanquake)
Pull request description:
Followup to #27447.
See https://releases.llvm.org/16.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html:
> Libc++ provides a debug mode that enables special debugging checks meant to detect incorrect usage of the standard library. These checks are disabled by default, but they can be enabled by vendors when building the library by using LIBCXX_ENABLE_DEBUG_MODE.
ACKs for top commit:
MarcoFalke:
lgtm ACK 4de9c2a65f
Tree-SHA512: 788c7f031ccf7a6ac96a87758e57f604cf4d9db0144f0ecc4931823111d2396e64ab260825d74f06b2770d0ac3e4e2c21c46f4def046cf3e1a44d705921ab6d2
bc4fd49d09 depends: add _LIBCPP_ENABLE_ASSERTIONS to DEBUG mode (fanquake)
cf266b2270 depends: Remove _LIBCPP_DEBUG from depends DEBUG mode (fanquake)
Pull request description:
It was deprecated in LLVM 15, turned into compile-time error in LLVM 16:
```bash
In file included from /usr/lib/llvm-16/bin/../include/c++/v1/cassert:19:
/usr/lib/llvm-16/bin/../include/c++/v1/__assert:22:5: error: "Defining _LIBCPP_DEBUG is not supported anymore.
Please use _LIBCPP_ENABLE_DEBUG_MODE instead."
^
1 error generated.
```
and has been removed entirely in LLVM 17 (main): ff573a42cd.
[Building libc++ in debug mode](https://releases.llvm.org/16.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html), will also automatically set
`_LIBCPP_ENABLE_DEBUG_MODE` (the new define), so adding it to depends
doesn't seem useful, and would just result in redefinition errors.
I'm wondering if as a followup, we could enable a DEBUG build of libc++
in our MSAN CI job? i.e https://github.com/fanquake/bitcoin/tree/msan_with_enable_debug_mode.
Somewhat related to https://github.com/google/oss-fuzz/pull/9828, where
it looks like we'll have to sort out getting a DEBUG build of LLVM, and can drop the commentary about re-enabling DEBUG=1.
ACKs for top commit:
MarcoFalke:
lgtm Approach ACK bc4fd49d09
Tree-SHA512: 9c0f48fc428278fbf34fbb8f81e761e232506d7ab28e971cb9a9b9a81d549b4d8bbe51e2f7608d56e489428679231da5b7431443849b238a8a993ad241740282
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
