🤔 furszy reviewed a pull request: "wallet: don't duplicate change output if already exist"
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1537117279)
Updated per feedback. Thanks.
Added docs for the `ComputeChangeParams` new function.
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1537117279)
Updated per feedback. Thanks.
Added docs for the `ComputeChangeParams` new function.
📝 furszy converted_to_draft a pull request: "wallet: coverage for receiving txes with same id but different witness data"
(https://github.com/bitcoin/bitcoin/pull/25909)
Based on #11240 context, adding test coverage for the behavior introduced in #11225 and to the current wallet limitations.
This is the first step towards adding the ability to store multiple transactions with same tx id but different witness data in the wallet. Verifying and testing the current behavior before introducing the new features.
The following cases are covered:
1) Two p2wpkh spending transactions with the same hash are received:
The first one with segwit data stripped, and t
...
(https://github.com/bitcoin/bitcoin/pull/25909)
Based on #11240 context, adding test coverage for the behavior introduced in #11225 and to the current wallet limitations.
This is the first step towards adding the ability to store multiple transactions with same tx id but different witness data in the wallet. Verifying and testing the current behavior before introducing the new features.
The following cases are covered:
1) Two p2wpkh spending transactions with the same hash are received:
The first one with segwit data stripped, and t
...
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268106550)
In the latest code it is:
```cpp
socklen_t len = sizeof(addrun);
if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) {
```
feel free to ditch the `len` variable and use `sizeof(addrun)` directly when calling `ConnectToSocket()`.
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268106550)
In the latest code it is:
```cpp
socklen_t len = sizeof(addrun);
if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) {
```
feel free to ditch the `len` variable and use `sizeof(addrun)` directly when calling `ConnectToSocket()`.
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268107378)
I wouldn't use hardcode default values here, but instead use the globals (e.g. `DEFAULT_TXRECONCILIATION_ENABLE`), and update the logic in `peerman_args.cpp` to only override Options members _if_ they are set as cli args.
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268107378)
I wouldn't use hardcode default values here, but instead use the globals (e.g. `DEFAULT_TXRECONCILIATION_ENABLE`), and update the logic in `peerman_args.cpp` to only override Options members _if_ they are set as cli args.
🤔 furszy reviewed a pull request: "init: return error when block index is non-contiguous, fix feature_init.py file perturbation"
(https://github.com/bitcoin/bitcoin/pull/27823#pullrequestreview-1535880019)
Code ACK d27b9a224
(https://github.com/bitcoin/bitcoin/pull/27823#pullrequestreview-1535880019)
Code ACK d27b9a224
💬 furszy commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1267289758)
Maybe could randomize (or restructure) this a bit in a follow-up. Perturbing different parts of the block content might let us detect and add coverage for different errors.
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1267289758)
Maybe could randomize (or restructure) this a bit in a follow-up. Perturbing different parts of the block content might let us detect and add coverage for different errors.
💬 furszy commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1268112906)
nit: couldn't this be copied outside of the for-loop only once?
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1268112906)
nit: couldn't this be copied outside of the for-loop only once?
💬 jamesob commented on pull request "test: Add missing `set -ex` to ci/lint/06_script.sh":
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806)
Only downside with this is that CI won't report all lint failures, rather will bail on the first one. Not sure if this is going to be annoying for people. Maybe we could have a `FAIL_FAST` envvar that gates this, and right now is only enabled in the container entrypoint script?
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806)
Only downside with this is that CI won't report all lint failures, rather will bail on the first one. Not sure if this is going to be annoying for people. Maybe we could have a `FAIL_FAST` envvar that gates this, and right now is only enabled in the container entrypoint script?
💬 fanquake commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1642153266)
> So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?
We don't currently have an easy way to use separate time-machines for differents HOSTS, and I'm not sure if that's something we'd want to do. The general time-machine bump is only blocked on one last issue, so that should be done shortly.
> (I mean, obviously my preference would be to use clang for Windows cross-builds,
I wouldn't be opposed to that. Although note that in tha
...
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1642153266)
> So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?
We don't currently have an easy way to use separate time-machines for differents HOSTS, and I'm not sure if that's something we'd want to do. The general time-machine bump is only blocked on one last issue, so that should be done shortly.
> (I mean, obviously my preference would be to use clang for Windows cross-builds,
I wouldn't be opposed to that. Although note that in tha
...
📝 dergoegge opened a pull request: "rfc: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107)
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only t
...
(https://github.com/bitcoin/bitcoin/pull/28107)
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only t
...
💬 MarcoFalke commented on pull request "test: Add missing `set -ex` to ci/lint/06_script.sh":
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268140307)
A fail-fast setting would also need to be picked up by lint-all to abort early, because this one does collect the failures.
But I won't be working on that soon, so should I just close this pull?
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268140307)
A fail-fast setting would also need to be picked up by lint-all to abort early, because this one does collect the failures.
But I won't be working on that soon, so should I just close this pull?
💬 MarcoFalke commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642175486)
How is this different from `GenTxid`?
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642175486)
How is this different from `GenTxid`?
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268148641)
We have already checked if UNIX sockets are supported.
```cpp
#if HAVE_SOCKADDR_UN
argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:' if UNIX domain sockets are supported by the platform.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
#else
argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268148641)
We have already checked if UNIX sockets are supported.
```cpp
#if HAVE_SOCKADDR_UN
argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:' if UNIX domain sockets are supported by the platform.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
#else
argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion
...
💬 dergoegge commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642196812)
> How is this different from GenTxid?
I think mostly compile-time vs. runtime checks? `GenTxid` can hold both txid types but doesn't enforce type correctness at compile time. Any type checks would happen at runtime, e.g. `assert(gtxid.IsWtxid())`. Also nothing prevents passing the wrong txid type when constructing a `GenTxid`.
(I guess with this PR we could consider making `GenTxid` a `std::variant<Txid, Wtxid>`)
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642196812)
> How is this different from GenTxid?
I think mostly compile-time vs. runtime checks? `GenTxid` can hold both txid types but doesn't enforce type correctness at compile time. Any type checks would happen at runtime, e.g. `assert(gtxid.IsWtxid())`. Also nothing prevents passing the wrong txid type when constructing a `GenTxid`.
(I guess with this PR we could consider making `GenTxid` a `std::variant<Txid, Wtxid>`)
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268176279)
Boy I was really scratching my head over this for like an hour ("how does this work at all?") then I remember the genesis block is written to a file when block count is still 0. So that's how we are rounding up.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268176279)
Boy I was really scratching my head over this for like an hour ("how does this work at all?") then I remember the genesis block is written to a file when block count is still 0. So that's how we are rounding up.
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268183969)
hm see https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1239761122 not sure what to do
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268183969)
hm see https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1239761122 not sure what to do
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268194341)
> I also ran a small unit test through perf where there were many writes and found that std::ftell dominated with std::fwrite close behind
For me fwrite dominated over ftell, but the most time was spent on XOR:
```
ROUTINE ====================== AutoFile::write in src/streams.cpp
49 100 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Spa
...
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268194341)
> I also ran a small unit test through perf where there were many writes and found that std::ftell dominated with std::fwrite close behind
For me fwrite dominated over ftell, but the most time was spent on XOR:
```
ROUTINE ====================== AutoFile::write in src/streams.cpp
49 100 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Spa
...
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268195960)
I am hoping to nuke `ftell` completely, but that would require removing the `Get` member function
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268195960)
I am hoping to nuke `ftell` completely, but that would require removing the `Get` member function
🤔 ismaelsadeeq reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1537066023)
Light code review ACK for commit 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6:
As I continue familiarizing myself with the code, my understanding is that this PR addresses follow-up nits up to a3b178f3c082453120e816945cea220bc7397ee5. Additionally, it introduces the use of `MiniMiner` to calculate the necessary fees required for fee bumping unconfirmed inputs to a target fee rate. The calculated fee bumps values are then added to the transaction's fee to account for the ancestor fee bump and reac
...
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1537066023)
Light code review ACK for commit 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6:
As I continue familiarizing myself with the code, my understanding is that this PR addresses follow-up nits up to a3b178f3c082453120e816945cea220bc7397ee5. Additionally, it introduces the use of `MiniMiner` to calculate the necessary fees required for fee bumping unconfirmed inputs to a target fee rate. The calculated fee bumps values are then added to the transaction's fee to account for the ancestor fee bump and reac
...
💬 ismaelsadeeq commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268063062)
We can use `CFeeRate(transaction_iter.value()->GetModFeesWithAncestors(), transaction_iter.value()->GetSizeWithAncestors())` to get the ancestor fee rate why are we calculating manually?
Like here ```tx6_anc_feerate = CFeeRate(high_fee + low_fee + med_fee, tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[6]);``` and other places?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268063062)
We can use `CFeeRate(transaction_iter.value()->GetModFeesWithAncestors(), transaction_iter.value()->GetSizeWithAncestors())` to get the ancestor fee rate why are we calculating manually?
Like here ```tx6_anc_feerate = CFeeRate(high_fee + low_fee + med_fee, tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[6]);``` and other places?