📝 hebasto opened a pull request: "Update translation source file for v28.0 string freeze"
(https://github.com/bitcoin-core/gui/pull/833)
This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to the [Release schedule for 28.0](https://github.com/bitcoin/bitcoin/issues/29891).
Note for reviewers: it is expected to get a zero diff after running `make -C src translate` locally.
(https://github.com/bitcoin-core/gui/pull/833)
This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to the [Release schedule for 28.0](https://github.com/bitcoin/bitcoin/issues/29891).
Note for reviewers: it is expected to get a zero diff after running `make -C src translate` locally.
💬 hebasto commented on pull request "Update translation source file for v28.0 string freeze":
(https://github.com/bitcoin-core/gui/pull/833#issuecomment-2285892290)
Friendly ping @stickies-v @pablomartin4btc :)
(https://github.com/bitcoin-core/gui/pull/833#issuecomment-2285892290)
Friendly ping @stickies-v @pablomartin4btc :)
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1715077813)
> It's hard to imagine this function being used for anything other than as a preprocessor to `FromHex`
The reason I carved it out instead of having it be a `FromHex()` parameter is not for reusability but to not bloat `FromHex()` with an explosion of input validation options (which also leaks into functions wrapping `base_blob::FromHex()`, such as `transaction_identifier::FromHex()`). I don't think there is a single answer to what we'll ever need for e.g. the below options, it can very much v
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1715077813)
> It's hard to imagine this function being used for anything other than as a preprocessor to `FromHex`
The reason I carved it out instead of having it be a `FromHex()` parameter is not for reusability but to not bloat `FromHex()` with an explosion of input validation options (which also leaks into functions wrapping `base_blob::FromHex()`, such as `transaction_identifier::FromHex()`). I don't think there is a single answer to what we'll ever need for e.g. the below options, it can very much v
...
👍 pablomartin4btc approved a pull request: "Update translation source file for v28.0 string freeze"
(https://github.com/bitcoin-core/gui/pull/833#pullrequestreview-2235263496)
ACK 9edd8f300ca27cfabc310cbe015dc14160d24f1a
Followed [instructions](https://github.com/bitcoin-core/gui/pull/793#issuecomment-1943421925) resulting in no diff as expected.
(https://github.com/bitcoin-core/gui/pull/833#pullrequestreview-2235263496)
ACK 9edd8f300ca27cfabc310cbe015dc14160d24f1a
Followed [instructions](https://github.com/bitcoin-core/gui/pull/793#issuecomment-1943421925) resulting in no diff as expected.
💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2286009740)
> I split `getCoinbaseMerklePath()` and `submitSolution()` out of this PR and moved them to [Sjors#53](https://github.com/Sjors/bitcoin/pull/53).
I've since `getCoinbaseMerklePath` in that PR to reuse `BlockMerkleBranch` instead of bringing its own `GetCoinBaseMerklePath` implementation. I'll move/reopen https://github.com/Sjors/bitcoin/pull/53 to the main repo after this PR is merged. I think it makes sense to keep it separate, because it requires a bit more mining specific knowledge to revi
...
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2286009740)
> I split `getCoinbaseMerklePath()` and `submitSolution()` out of this PR and moved them to [Sjors#53](https://github.com/Sjors/bitcoin/pull/53).
I've since `getCoinbaseMerklePath` in that PR to reuse `BlockMerkleBranch` instead of bringing its own `GetCoinBaseMerklePath` implementation. I'll move/reopen https://github.com/Sjors/bitcoin/pull/53 to the main repo after this PR is merged. I think it makes sense to keep it separate, because it requires a bit more mining specific knowledge to revi
...
🤔 hodlinator reviewed a pull request: "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`"
(https://github.com/bitcoin/bitcoin/pull/30619#pullrequestreview-2235136186)
Reviewed 6d7bf8ea8c11d95ae1cd5b803c151d07cdc2f8d6
Long-time devs state how MacOS is not a supported development platform, and this change does make things more verbose, so I'm curious to get their take. But I'm happy to see there is a way that works for both.
Verified that it does seem to be the right identifier to query by reading: https://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html
You are not changing **doc/fuzzing.md** which was linked in https://github.com/b
...
(https://github.com/bitcoin/bitcoin/pull/30619#pullrequestreview-2235136186)
Reviewed 6d7bf8ea8c11d95ae1cd5b803c151d07cdc2f8d6
Long-time devs state how MacOS is not a supported development platform, and this change does make things more verbose, so I'm curious to get their take. But I'm happy to see there is a way that works for both.
Verified that it does seem to be the right identifier to query by reading: https://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html
You are not changing **doc/fuzzing.md** which was linked in https://github.com/b
...
💬 hodlinator commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#discussion_r1715040805)
Typo "Or is" -> "Or if", also missing empty line before next block. My suggestion:
```suggestion
Linux has the less verbose:
```sh
make -j$(nproc)
```
(https://github.com/bitcoin/bitcoin/pull/30619#discussion_r1715040805)
Typo "Or is" -> "Or if", also missing empty line before next block. My suggestion:
```suggestion
Linux has the less verbose:
```sh
make -j$(nproc)
```
💬 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.