💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2286098751)
`11364e2e58..2db437fbd1`: do the transaction send with `INV+GETDATA+TX` instead of just `TX`, so that this PR is now not related or tied to #30572.
I think this complicates the code without providing a benefit, thus my preference is to avoid it, so I kept the change in separate commits (the two on the top of the branch). It can be reviewed separately and squashed accordingly if people like it, or easily dropped.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2286098751)
`11364e2e58..2db437fbd1`: do the transaction send with `INV+GETDATA+TX` instead of just `TX`, so that this PR is now not related or tied to #30572.
I think this complicates the code without providing a benefit, thus my preference is to avoid it, so I kept the change in separate commits (the two on the top of the branch). It can be reviewed separately and squashed accordingly if people like it, or easily dropped.
💬 vasild commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2286099551)
`INV+GETDATA` is now implemented in #29415 so that it does not stand in the way of this PR, but:
#21224 was frowned upon for a few reasons and one of them was breaking existent tools. This PR addresses just that by introducing a new protocol version. It does not address the other concerns.
On the new protocol idea - we don't expect that an attacker will voluntarily switch to the new protocol so that their attack can be thwarted, right? If this PR is merged and at some point in the future,
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2286099551)
`INV+GETDATA` is now implemented in #29415 so that it does not stand in the way of this PR, but:
#21224 was frowned upon for a few reasons and one of them was breaking existent tools. This PR addresses just that by introducing a new protocol version. It does not address the other concerns.
On the new protocol idea - we don't expect that an attacker will voluntarily switch to the new protocol so that their attack can be thwarted, right? If this PR is merged and at some point in the future,
...
👍 tdb3 approved a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235393961)
ACK 701530045553f2b9671a3fffea301bf4dc954514
Good finds.
Ran `bitcoin-cli help getblockchaininfo` and `help getmininginfo` and confirmed presence of `testnet4`.
Left a non-blocking comment. Would be beneficial to de-deplicate.
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235393961)
ACK 701530045553f2b9671a3fffea301bf4dc954514
Good finds.
Ran `bitcoin-cli help getblockchaininfo` and `help getmininginfo` and confirmed presence of `testnet4`.
Left a non-blocking comment. Would be beneficial to de-deplicate.
💬 tdb3 commented on pull request "doc: add missing "testnet4" network string in RPC/init help texts":
(https://github.com/bitcoin/bitcoin/pull/30642#discussion_r1715195404)
Yes, this list of chain names could be de-duplicated as a macro as maflcko suggested or something like a `std::string ListChainTypes()` (perhaps in src/util/chaintypes.h/cpp, src/chainparamsbase.h/cpp, src/common/messages.h/cpp, etc. or where appropriate).
(https://github.com/bitcoin/bitcoin/pull/30642#discussion_r1715195404)
Yes, this list of chain names could be de-duplicated as a macro as maflcko suggested or something like a `std::string ListChainTypes()` (perhaps in src/util/chaintypes.h/cpp, src/chainparamsbase.h/cpp, src/common/messages.h/cpp, etc. or where appropriate).
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1715214130)
8523d19bb3f8c982c6e01a4635c98cd9ac386dea: I wonder if this causes a problem when handling a block template without a witness commitment.
If so, I might just have stratum v2 templates always set a coinbase witness.
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1715214130)
8523d19bb3f8c982c6e01a4635c98cd9ac386dea: I wonder if this causes a problem when handling a block template without a witness commitment.
If so, I might just have stratum v2 templates always set a coinbase witness.
🤔 glozow reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2232929841)
light code review ACK a0abcbd3822
I'm unfamiliar with the descriptor code, but the refactoring commits look like pure refactoring, behavior change commits look correct, RPC changes seem intuitive, and tests seem to cover things when I introduce mutations.
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2232929841)
light code review ACK a0abcbd3822
I'm unfamiliar with the descriptor code, but the refactoring commits look like pure refactoring, behavior change commits look correct, RPC changes seem intuitive, and tests seem to cover things when I introduce mutations.
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1713711352)
Isn't this 3?
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1713711352)
Isn't this 3?
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715130340)
nit "second elements". Also could be helpful that apart from this special case, all multipath are non-internal
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715130340)
nit "second elements". Also could be helpful that apart from this special case, all multipath are non-internal
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715147527)
(I don't know how frequently we expect each type of usage to be.) Did you consider returning single derivation descriptors in both result arrays, so that the original result can eventually be removed (after deprecation, everyone adds a [0] to their scripts when using single)?
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715147527)
(I don't know how frequently we expect each type of usage to be.) Did you consider returning single derivation descriptors in both result arrays, so that the original result can eventually be removed (after deprecation, everyone adds a [0] to their scripts when using single)?
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715114909)
Could also add a comment here on the `j==1`, "first is receiving, second is change" or something
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715114909)
Could also add a comment here on the `j==1`, "first is receiving, second is change" or something
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715151078)
Can `Assume(!multipath_values.empty())` here
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715151078)
Can `Assume(!multipath_values.empty())` here
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715150336)
Could add some comments for future wallet reviewers, maybe mentioning that they should be consistent with each other (multipath_segment_index.has_value() == !multipath_values.empty() == !seen_multipath.empty())
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715150336)
Could add some comments for future wallet reviewers, maybe mentioning that they should be consistent with each other (multipath_segment_index.has_value() == !multipath_values.empty() == !seen_multipath.empty())
💬 paplorinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#discussion_r1715243406)
Done, thanks!
These files are also edited during the CMake migration, we can finalize the details once that's merged.
(https://github.com/bitcoin/bitcoin/pull/30619#discussion_r1715243406)
Done, thanks!
These files are also edited during the CMake migration, we can finalize the details once that's merged.
💬 furszy commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1715277603)
> What compiler? I'm not seeing this error, and it seems neither did CI.
clang 14.0.3 (clang-1403.0.22.14.1).
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1715277603)
> What compiler? I'm not seeing this error, and it seems neither did CI.
clang 14.0.3 (clang-1403.0.22.14.1).
💬 hodlinator commented on pull request "refactor: Add consteval ArrayFromBytes()":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715287560)
Went with `ArrayFromBytes` for all in the latest tip d6d7a0b5221935518d2797aec7abc5c9632cbf68.
Avoided adding a public `CScript::operator<<(std::span)` and went for a private method + public `std::array` overload.
Have a variant with consteval validation in VecFromHex-branch, but didn't feel it was urgent to use in any of the cases, so currently dropped from this PR:
https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715287560)
Went with `ArrayFromBytes` for all in the latest tip d6d7a0b5221935518d2797aec7abc5c9632cbf68.
Avoided adding a public `CScript::operator<<(std::span)` and went for a private method + public `std::array` overload.
Have a variant with consteval validation in VecFromHex-branch, but didn't feel it was urgent to use in any of the cases, so currently dropped from this PR:
https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110
👋 hodlinator's pull request is ready for review: "refactor: Add consteval ArrayFromBytes()"
(https://github.com/bitcoin/bitcoin/pull/30377)
(https://github.com/bitcoin/bitcoin/pull/30377)
💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715292834)
I expect single path to still be most used and I thought that returning an array for that would be confusing.
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715292834)
I expect single path to still be most used and I thought that returning an array for that would be confusing.
💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2286256207)
@pythcoiner @mjdietzx Can you please re-review?
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2286256207)
@pythcoiner @mjdietzx Can you please re-review?
💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715303747)
Makes sense
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715303747)
Makes sense
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2286346598)
`2db437fbd1...df3a22f87a`: pet clang-tidy and combine the last two commits (that implement the `INV+GETDATA`) because otherwise the test-each-commit CI task fails.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2286346598)
`2db437fbd1...df3a22f87a`: pet clang-tidy and combine the last two commits (that implement the `INV+GETDATA`) because otherwise the test-each-commit CI task fails.