c4981e7f63 prune, import: fixes#23852 (mruddy)
Pull request description:
Fixes#23852
This allows pruning to work during the `-loadblock` import process.
An example use case is where you have a clean set of block files and you want to create a pruned node from them, but you don't want to alter the input set of block files.
#23852 noted that pruning was not working reliably during the loadblock import process. The reason why the loadblock process was not pruning regularly as it progressed is that the pruning process (`BlockManager::FindFilesToPrune`) checks the tip height of the active chainstate, and `CChainState::ActivateBestChain` was not called (which updates that tip height) in `ThreadImport` until after all the import files were processed.
An example bash command line that makes it easy to import a bunch of block files:
```
./src/qt/bitcoin-qt -debug -logthreadnames -datadir=/tmp/btc -prune=550 -loadblock=/readonly/btc/main/blk{00000..00043}.dat
```
One interesting side note is that `CChainState::ActivateBestChain` can be called while the import process is running (in the `loadblk` thread) by concurrent network message processing activity in the `msghand` thread. For example, one way to reproduce this easily is with the `getblockfrompeer` RPC (requesting a block with height greater than 100000) run from a node connected to an importing node. There are other ways too, but this is an easy way. I only mention this to explain how the `max_prune_height=225719` log message in the original issue came to occur.
ACKs for top commit:
achow101:
re-ACK c4981e7f63
Tree-SHA512: d287c7753952c22f598ba782914c47f45ad44ce60b0fbce9561354e701f1a2a98bafaaaa106c8428690b814e281305ca3622b177ed3cb2eb7559f07c958ab537
daba95700b refactor: Make ListSelected return vector (Sebastian Falbesoner)
94776621ba wallet: Move CoinCointrol definitions to .cpp (Aurèle Oulès)
1db23da6e1 wallet: Use std::optional for GetExternalOutput and fixups (Aurèle Oulès)
becc45b589 scripted-diff: Rename setSelected->m_selected_inputs (Aurèle Oulès)
Pull request description:
- Moves CoinControl function definitions from `coincontrol.h` to `coincontrol.cpp`
- Adds more documentation
- Renames class member for an improved comprehension
- Use `std::optional` for `GetExternalOutput`
ACKs for top commit:
achow101:
ACK daba95700b
Xekyo:
ACK daba95700b
Tree-SHA512: 3bf2dc834a3246c2f53f8c55154258e605fcb169431d3f7b156931f33c7e3b1ae28e03e16b37f9140a827890eb7798be485b2c36bfc23ff29bb01763f289a07c
Under which circumstances we process received 'mempool' P2P messages
caused confusion in #27426. Rather than bikeshedding the formulation
of the IF-statement, this adds a comment clarifing when we process
the message. Also, correcting the comment of `m_send_mempool`.
Co-authored-by: willcl-ark <will8clark@gmail.com>
710b83938a rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs (Harris)
Pull request description:
Reopens#18570 and closes#18567.
I have rebased the original PR.
Not sure why the original got closed as it was about to get merged.
ACKs for top commit:
achow101:
ACK 710b83938a
Tree-SHA512: d4478d990be98b1642e9ffb2930987f4a224e8bd64e2e35a5dda927a54c509ec9d712cd0eac35dc2bb89f00a1678e530ce14d7445f1dd93aa3a4cce2bc9b130d
8f14fc8622 test: cover fastprune with excessive block size (Matthew Zipkin)
271c23e87f blockstorage: Adjust fastprune limit if block exceeds blockfile size (Martin Zumsande)
Pull request description:
The debug-only `-fastprune` option used in several tests is not always safe to use:
If a `-fastprune` node receives a block larger than the maximum blockfile size of `64kb` bad things happen: The while loop in `BlockManager::FindBlockPos` never terminates, and the node runs oom because memory for `m_blockfile_info` is allocated in each iteration of the loop.
The same would happen if a naive user used `-fastprune` on anything other than regtest (so this can be tested by syncing on signet for example, the first block that crashes the node is at height 2232).
Change the approach by raising the blockfile size to the size of the block, if that block otherwise wouldn't fit (idea by TheCharlatan).
ACKs for top commit:
ryanofsky:
Code review ACK 8f14fc8622. Added new assert, test, and comment since last review
TheCharlatan:
ACK 8f14fc8622
pinheadmz:
ACK 8f14fc8622
Tree-SHA512: df2fea30613ef9d40ebbc2416eacb574f6d7d96847db5c33dda22a29a2c61a8db831aa9552734ea4477e097f253dbcb6dcb1395d43d2a090cc0588c9ce66eac3
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.
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
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
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
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
If the added block exceeds the blockfile size in test-only
-fastprune mode, the node would get stuck in an infinite loop and
run out of memory.
Avoid this by raising the blockfile size to the size of the added block
in this situation.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
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.
0d6383fda0 Don't return OutputType::UNKNOWN in ParseOutputType (Pttn)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/27472 by also handling at the relevant places the case where ParseOutputType returns `OutputType::UNKNOWN`, and not just when it returns `std::nullopt`.
ACKs for top commit:
achow101:
ACK 0d6383fda0
MarcoFalke:
lgtm ACK 0d6383fda0
furszy:
ACK 0d6383fda0
Tree-SHA512: 776793027b926283d3162e69fb9c8883c814b19bcce4574ccdf8e3140a1ec4ebc4aa8ccd1abae7ef3571f942d2e6c35305fd1244259540d90605106e01afc34c
`evhttp_uri_parse` can return a nullptr, for example when the URI
contains invalid characters (e.g. "%").
`GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
straight into `evhttp_uri_get_query`, which means that anyone calling
a REST endpoint in which query parameters are used (e.g. `rest_headers`)
can cause a segfault.
This bugfix is designed to be minimal and without additional behaviour change.
Follow-up work should be done to resolve this in a more general and robust way,
so not every endpoint has to handle it individually.
Avoid adding transactions below min relay feerate because, even if they
were bumped through CPFP when entering the mempool, we do not have a
DoS-resistant way of ensuring they always remain bumped. In the future,
this rule can be relaxed (e.g. to allow packages to bump 0-fee
transactions) if we find a way to do so.
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 that 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.
d52fa1b0a5 tests: Make sure that bumpfee feerate checks work when replacing outputs (Andrew Chow)
be177c15a4 bumpfee: Check the correct feerate when replacing outputs (Andrew Chow)
Pull request description:
When replacing the outputs of a transaction during `bumpfee`, it is possible to accidentally create a transaction that will not be accepted into the mempool as it does not meet the incremental relay fee requirements. This occurs because the size estimation used for checking the provided feerate does not account for the replaced outputs; it instead uses the original outputs. When the replaced outputs is significantly different from the original, there can be a large difference in estimated transaction sizes that can make a transaction miss the absolute fee requirements for the incremental relay fee. Unfortunately we do not currently inform the user when the bumped transaction fails to relay, so they could use `bumpfee` and think the transaction has been bumped when it actually has not.
This issue is resolved by replacing the outputs before doing the size estimation, and also updating the feerate checker to use the actual fee values when calculating the required minimum fee.
Also added a test for this scenario.
ACKs for top commit:
ishaanam:
reACK d52fa1b0a5
Xekyo:
reACK d52fa1b0a5
Tree-SHA512: d18301b587465322dd3fb1bb86496c3675265a56072047576e2baa5cf907dd3b54778f30721f662f0c235709a5568427c18542eb7efbfb6fdd9f481fe676c66b
4258c54f4e Merge bitcoin-core/secp256k1#1276: autotools: Don't regenerate Wycheproof header automatically
06c67dea9f autotools: Don't regenerate Wycheproof header automatically
3bab71cf05 Merge bitcoin-core/secp256k1#1268: release cleanup: bump version after 0.3.1
656c6ea8d8 release cleanup: bump version after 0.3.1
346a053d4c Merge bitcoin-core/secp256k1#1269: changelog: Fix link
6a37b2a5ea changelog: Fix link
ec98fcedd5 Merge bitcoin-core/secp256k1#1266: release: Prepare for 0.3.1
898e1c676e release: Prepare for 0.3.1
1d9a13fc26 changelog: Remove inconsistent newlines
0e091669a1 changelog: Catch up in preparation of 0.3.1
7b7503dac5 Merge bitcoin-core/secp256k1#1245: tests: Add Wycheproof ECDSA vectors
145078c418 Merge bitcoin-core/secp256k1#1118: Add x-only ecmult_const version with x specified as n/d
e5de454609 tests: Add Wycheproof ECDSA vectors
0f8642079b Add exhaustive tests for ecmult_const_xonly
4485926ace Add x-only ecmult_const version for x=n/d
a0f4644f7e Merge bitcoin-core/secp256k1#1252: Make position of * in pointer declarations in include/ consistent
4e682626a3 Merge bitcoin-core/secp256k1#1226: Add CMake instructions to release process
2d51a454fc Merge bitcoin-core/secp256k1#1257: ct: Use volatile "trick" in all fe/scalar cmov implementations
4a496a36fb ct: Use volatile "trick" in all fe/scalar cmov implementations
3d1f430f9f Make position of * in pointer declarations in include/ consistent
2bca0a5cbf Merge bitcoin-core/secp256k1#1241: build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro
afd8b23b27 Merge bitcoin-core/secp256k1#1244: Suppress `-Wunused-parameter` when building for coverage analysis
1d8f367515 Merge bitcoin-core/secp256k1#1250: No need to subtract 1 before doing a right shift
3e43041be6 No need to subtract 1 before doing a right shift
3addb4c1e8 build: Improve `SECP_TRY_APPEND_DEFAULT_CFLAGS` macro
0c07c82834 Add CMake instructions to release process
464a9115b4 Merge bitcoin-core/secp256k1#1242: Set ARM ASM symbol visibility to `hidden`
f16a709fd6 Merge bitcoin-core/secp256k1#1247: Apply Checks only in VERIFY mode.
70be3cade5 Merge bitcoin-core/secp256k1#1246: Typo
4ebd82852d Apply Checks only in VERIFY mode.
d1e7ca192d Typo
5bb03c2911 Replace `SECP256K1_ECMULT_TABLE_VERIFY` macro by a function
9c8c4f443c Merge bitcoin-core/secp256k1#1238: build: bump CMake minimum requirement to 3.13
0cf2fb91ef Merge bitcoin-core/secp256k1#1243: build: Ensure no optimization when building for coverage analysis
fd2a408647 Set ARM ASM symbol visibility to `hidden`
4429a8c218 Suppress `-Wunused-parameter` when building for coverage analysis
8e79c7ed11 build: Ensure no optimization when building for coverage analysis
96dd062511 build: bump CMake minimum requirement to 3.13
427bc3cdcf Merge bitcoin-core/secp256k1#1236: Update comment for secp256k1_modinv32_inv256
647f0a5cb1 Update comment for secp256k1_modinv32_inv256
5658209459 Merge bitcoin-core/secp256k1#1228: release cleanup: bump version after 0.3.0
28e63f7ea7 release cleanup: bump version after 0.3.0
git-subtree-dir: src/secp256k1
git-subtree-split: 4258c54f4ebfc09390168e8a43306c46b315134b
b5585ba5f9 p2p: skip netgroup diversity of new connections for tor/i2p/cjdns networks (stratospher)
Pull request description:
Follow up for #27264.
In order to make sure that our persistent outbound slots belong to different netgroups, distinct net groups of our peers are added to `setConnected`. We’d only open a persistent outbound connection to peers which have a different netgroup compared to those netgroups present in `setConnected`.
Current `GetGroup()` logic assumes route-based diversification behaviour for tor/i2p/cjdns addresses (addresses are public key based and not route-based). Distinct netgroups possible (according to the current `GetGroup()` logic) for:
1. tor => 030f, 031f, .. 03ff (16 possibilities)
2. i2p => 040f, 041f, .. 04ff (16 possibilities)
3. cjdns => 05fc0f, 05fc1f, ... 05fcff (16 possibilities)
`setConnected` is used in `ThreadOpenConnections()` before making [outbound](84f4ac39fd/src/net.cpp (L1846)) and [anchor](84f4ac39fd/src/net.cpp (L1805)) connections to new peers so that they belong to distinct netgroups.
**behaviour on master**
- if we run a node only on tor/i2p/cjdns
- we wouldn't be able to open more than 16 outbound connections(manual, block-relay-only anchor, outbound full relay, block-relay-only connections) because we run out of possible netgroups.
- see https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481322628
- tested by changing `MAX_OUTBOUND_FULL_RELAY_CONNECTIONS` to 17 with `onlynet=onion` and observed how node wouldn't make more than 16 outbound connections.
**behaviour on PR**
- netgroup diversity checks are skipped for tor/i2p/cjdns addresses.
- we don't insert tor/i2p/cjdns address in `setConnected` and `GetGroup` doesn't get called on tor/i2p/cjdns(see #27369)
ACKs for top commit:
achow101:
ACK b5585ba5f9
mzumsande:
ACK b5585ba5f9
vasild:
ACK b5585ba5f9
Tree-SHA512: c120b3f9ca7f0be3f29ea665cd2f7dfb40cd1d7ec7058984252fb6e0295e414f736c5b4fba03c31188188a5ae4f543fb2654f6ee9776bad745c7ca72d23d5b9b
