📝 achow101 converted_to_draft a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467)
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.
This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify t
...
(https://github.com/bitcoin/bitcoin/pull/26467)
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.
This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify t
...
📝 pinheadmz opened a pull request: "system: cache config file path before potentially updating datadir"
(https://github.com/bitcoin/bitcoin/pull/27303)
Fixes an issue where a `bitcoin.conf` file that contains a `datadir=` setting would be incorrectly reported in the logs (see examples below). This fix is implemented by caching the path to the conf file before setting `datadir`.
## Master
```
Default data directory /Users/matthewzipkin/Library/Application Support/Bitcoin
Using data directory /tmp/bcdata/regtest
Config file: /tmp/bcdata/bitcoin.conf (not found, skipping)
Config file arg: blocksdir="/tmp/blocks"
Config file arg: datadir
...
(https://github.com/bitcoin/bitcoin/pull/27303)
Fixes an issue where a `bitcoin.conf` file that contains a `datadir=` setting would be incorrectly reported in the logs (see examples below). This fix is implemented by caching the path to the conf file before setting `datadir`.
## Master
```
Default data directory /Users/matthewzipkin/Library/Application Support/Bitcoin
Using data directory /tmp/bcdata/regtest
Config file: /tmp/bcdata/bitcoin.conf (not found, skipping)
Config file arg: blocksdir="/tmp/blocks"
Config file arg: datadir
...
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145326660)
2fa935b389ac05c35 As `getaddrmaninfo` would be a new RPC, this code would break `-addrinfo` for clients using this code to call to long-running nodes running earlier versions of bitcoind starting from v22 (#21595). Please maintain compatibility (not in output, just the call still working) with these versions in your approach.
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145326660)
2fa935b389ac05c35 As `getaddrmaninfo` would be a new RPC, this code would break `-addrinfo` for clients using this code to call to long-running nodes running earlier versions of bitcoind starting from v22 (#21595). Please maintain compatibility (not in output, just the call still working) with these versions in your approach.
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1145331263)
> If there isn't some more convenient way to review changes like this, I would definitely appreciate it if the code could just be moved and not reformatted.
ACK, I will stop using `clang-format-diff` on all moved code. Thank you for the review!
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1145331263)
> If there isn't some more convenient way to review changes like this, I would definitely appreciate it if the code could just be moved and not reformatted.
ACK, I will stop using `clang-format-diff` on all moved code. Thank you for the review!
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145335359)
`CLI -addrinfo previously only returned a subset of addresses` and referring to a hidden RPC might raise more questions than it answers here for users trying to figure out what's going on.
Maybe borrow from the current -addrinfo help, along the lines of `CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.`
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145335359)
`CLI -addrinfo previously only returned a subset of addresses` and referring to a hidden RPC might raise more questions than it answers here for users trying to figure out what's going on.
Maybe borrow from the current -addrinfo help, along the lines of `CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.`
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1480193552)
> ping @jonatack, could you have a look, especially at the changes to bitcoin-cli?
Thanks! Sorry for missing the ping and not looking sooner.
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1480193552)
> ping @jonatack, could you have a look, especially at the changes to bitcoin-cli?
Thanks! Sorry for missing the ping and not looking sooner.
💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145385658)
Testing the output of this RPC and -addrinfo side-by-side to check their output, I found myself mentally summing the RPC values to compare with the -addrinfo totals and thinking that it would be handy to have the sum returned by the RPC as well, i.e. new/tried/total.
```suggestion
"\nReturns the number of addresses in the `new` and `tried` tables and their sum for all networks (default) or a given network. This RPC is for testing only.\n",
```
```
$ ./src/bitcoin-c
...
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145385658)
Testing the output of this RPC and -addrinfo side-by-side to check their output, I found myself mentally summing the RPC values to compare with the -addrinfo totals and thinking that it would be handy to have the sum returned by the RPC as well, i.e. new/tried/total.
```suggestion
"\nReturns the number of addresses in the `new` and `tried` tables and their sum for all networks (default) or a given network. This RPC is for testing only.\n",
```
```
$ ./src/bitcoin-c
...
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145355201)
Unimplemented function
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145355201)
Unimplemented function
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145368125)
Strictly speaking, the descendant size can only be greater than the ancestor size.
```suggestion
Assume(descendant->second.vsize_with_ancestors > anc->second.GetTxSize());
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145368125)
Strictly speaking, the descendant size can only be greater than the ancestor size.
```suggestion
Assume(descendant->second.vsize_with_ancestors > anc->second.GetTxSize());
```
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145373839)
As the ancestors structure is a set and we are merely looping over it, I don't see how this can realistically happen. Two different iterators pointing to the same object? (if that is the worry, then the problem is with the iterators usage approach)
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145373839)
As the ancestors structure is a set and we are merely looping over it, I don't see how this can realistically happen. Two different iterators pointing to the same object? (if that is the worry, then the problem is with the iterators usage approach)
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145394065)
What is the reason behind this separate removal loop? Couldn't we merge it with the loop that is above?
Are you worry about the `ancestors` set having a parent and a child so if first removes the child then the parent will not find it and then crash?
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145394065)
What is the reason behind this separate removal loop? Couldn't we merge it with the loop that is above?
Are you worry about the `ancestors` set having a parent and a child so if first removes the child then the parent will not find it and then crash?
💬 willcl-ark commented on issue "contrib/install_db4.sh script fails on OpenBSD 7.2 (RPi 3) (error: Unable to find a mutex implementation)":
(https://github.com/bitcoin/bitcoin/issues/26860#issuecomment-1480275742)
@m-kubik were you able to get this working with depends? If so I think we can probably close this issue.
(https://github.com/bitcoin/bitcoin/issues/26860#issuecomment-1480275742)
@m-kubik were you able to get this working with depends? If so I think we can probably close this issue.
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145426398)
I think I'd lean here towards option 3, not breaking backwards-compatibility and just let json2.0 users "suffer" a `status: 500, error: -32700` :)
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145426398)
I think I'd lean here towards option 3, not breaking backwards-compatibility and just let json2.0 users "suffer" a `status: 500, error: -32700` :)
📝 ishaanam opened a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307)
The `TxStateMempoolConflicted` class is added. It is serialized the same as `TxStateInactive` and `TxStateInMempool`. This is done because mempool-related states are relatively short lived, so it doesn't make sense to serialize them differently than they are on master (mempool-conflicted txs are currently marked as `TxStateInactive`). As a result, this PR only changes how these transaction are dealt with in memory.
In memory `TxStateConflicted` (which only refers to block conflicts) and `TxSt
...
(https://github.com/bitcoin/bitcoin/pull/27307)
The `TxStateMempoolConflicted` class is added. It is serialized the same as `TxStateInactive` and `TxStateInMempool`. This is done because mempool-related states are relatively short lived, so it doesn't make sense to serialize them differently than they are on master (mempool-conflicted txs are currently marked as `TxStateInactive`). As a result, this PR only changes how these transaction are dealt with in memory.
In memory `TxStateConflicted` (which only refers to block conflicts) and `TxSt
...
💬 sipa commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145435338)
Alternatively, just round up size 0 to be at least alignment?
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145435338)
Alternatively, just round up size 0 to be at least alignment?
💬 wbobeirne commented on issue "Bitcoind repeatedly crashing at `UpdateTip` with no error message":
(https://github.com/bitcoin/bitcoin/issues/24483#issuecomment-1480313309)
@pinheadmz I'm fairly certain that despite SMART not reporting any issues, there was hardware disk failure at play here. I started to see instability in my system elsewhere. I haven't yet swapped the offending hard drive to confirm, but this can probably be closed.
(https://github.com/bitcoin/bitcoin/issues/24483#issuecomment-1480313309)
@pinheadmz I'm fairly certain that despite SMART not reporting any issues, there was hardware disk failure at play here. I started to see instability in my system elsewhere. I haven't yet swapped the offending hard drive to confirm, but this can probably be closed.
💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#issuecomment-1480339025)
Thank you for the reviews and the helpful and patient comments!
Updated 571c7bd983035e4b684f289d97f00542ac9dc117 -> 5d166cae8cd16959bac7527779540daf81c328e4 ([kernelChainName_0](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_0) -> [kernelChainName_1](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainName_0..kernelChainName_1)):
* Addressed @ajtowns [comment](https://github.com/bitcoin/bitc
...
(https://github.com/bitcoin/bitcoin/pull/27294#issuecomment-1480339025)
Thank you for the reviews and the helpful and patient comments!
Updated 571c7bd983035e4b684f289d97f00542ac9dc117 -> 5d166cae8cd16959bac7527779540daf81c328e4 ([kernelChainName_0](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_0) -> [kernelChainName_1](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainName_0..kernelChainName_1)):
* Addressed @ajtowns [comment](https://github.com/bitcoin/bitc
...
💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145478812)
@martinus, good catch!
@sipa, good idea, this does seem to work (hope this is what you meant):
```
[[nodiscard]] static constexpr std::size_t NumElemAlignBytes(std::size_t bytes)
{
- return (bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES;
+ return bytes > 0 ? ((bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES) : 1;
}
```
Benchmark results are identical for me, but this should be confirmed.
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145478812)
@martinus, good catch!
@sipa, good idea, this does seem to work (hope this is what you meant):
```
[[nodiscard]] static constexpr std::size_t NumElemAlignBytes(std::size_t bytes)
{
- return (bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES;
+ return bytes > 0 ? ((bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES) : 1;
}
```
Benchmark results are identical for me, but this should be confirmed.
💬 sipa commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145490940)
Yeah, or even:
```c++
return (bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES + (bytes == 0);
```
which might be a minuscule amount faster.
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145490940)
Yeah, or even:
```c++
return (bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES + (bytes == 0);
```
which might be a minuscule amount faster.
📝 achow101 opened a pull request: "bumpfee: avoid making bumped transactions with too low fee when replacing outputs"
(https://github.com/bitcoin/bitcoin/pull/27308)
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 transa
...
(https://github.com/bitcoin/bitcoin/pull/27308)
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 transa
...
💬 ProofOfKeags commented on issue "Manual-pruning cursor rewind":
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1480381691)
> would not be able to handle a deeper chain reorg than the original prune depth.
The smallest prune size is 550MB. If we have a 140 block reorg, we have a pretty apocalyptic scenario. afaiu there are numerous assumptions within bitcoin core that a 100 block reorg is impossible. Under that assumption, your described limitation isn't actually a limitation.
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1480381691)
> would not be able to handle a deeper chain reorg than the original prune depth.
The smallest prune size is 550MB. If we have a 140 block reorg, we have a pretty apocalyptic scenario. afaiu there are numerous assumptions within bitcoin core that a 100 block reorg is impossible. Under that assumption, your described limitation isn't actually a limitation.