💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020)
I didn't previously realize that this PR breaks use-facing compatibility wrt `0x` prefixes in args. Would also like to see such cases be filtered through `TrySanitizeHexNumber` before going into `FromHex` so that level of compatibility is maintained.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020)
I didn't previously realize that this PR breaks use-facing compatibility wrt `0x` prefixes in args. Would also like to see such cases be filtered through `TrySanitizeHexNumber` before going into `FromHex` so that level of compatibility is maintained.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2285769580)
`2b478bf332...11364e2e58`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2285769580)
`2b478bf332...11364e2e58`: rebase due to conflicts
🤔 hodlinator requested changes to a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2234995444)
Downgrade to Concept ACK 855784d3a0026414159acc42fceeb271f8a28133 in light of https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2234995444)
Downgrade to Concept ACK 855784d3a0026414159acc42fceeb271f8a28133 in light of https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020
🚀 glozow merged a pull request: "net: Clarify that m_addr_local is only set once"
(https://github.com/bitcoin/bitcoin/pull/30617)
(https://github.com/bitcoin/bitcoin/pull/30617)
🤔 glozow reviewed a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235009746)
ACK 701530045553f2b9671a3fffea301bf4dc954514
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235009746)
ACK 701530045553f2b9671a3fffea301bf4dc954514
💬 fjahr commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2285794512)
> If a few more people can review this in the next day or so, I believe we can have this in for 28.0.
I have pinged a few more people for (re-)reviews and testing but please give it until the end of the week because testing the full flow can take a while depending on what machine you are doing it on or what connection speed you have.
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2285794512)
> If a few more people can review this in the next day or so, I believe we can have this in for 28.0.
I have pinged a few more people for (re-)reviews and testing but please give it until the end of the week because testing the full flow can take a while depending on what machine you are doing it on or what connection speed you have.
👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2235025267)
Nice, ACK fa48ded21168740640ff9d6e5b413a322d58b114
I'd re-ack quickly if you want to fix the other includes as well.
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2235025267)
Nice, ACK fa48ded21168740640ff9d6e5b413a322d58b114
I'd re-ack quickly if you want to fix the other includes as well.
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013521)
Fixed (in the same way as `waitforblock`)
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013521)
Fixed (in the same way as `waitforblock`)
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013631)
Fixed, though in a slightly different way.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013631)
Fixed, though in a slightly different way.
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013711)
Done
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013711)
Done
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2285847403)
Re https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417
> it looks like this will wait 1000ms+1000ms instead of 1000ms+500ms
Indeed it seems to pick the wrong wait time after the first inner loop. If fixed that and the overflow, though in a slightly different way than the AI overlord suggested.
I also added a `g_best_block == uint256()` check because it starts 0 until the first `UpdateTip()`.
Changed current_tip argument to `uint256` instead of `BlockRef`.
I also
...
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2285847403)
Re https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417
> it looks like this will wait 1000ms+1000ms instead of 1000ms+500ms
Indeed it seems to pick the wrong wait time after the first inner loop. If fixed that and the overflow, though in a slightly different way than the AI overlord suggested.
I also added a `g_best_block == uint256()` check because it starts 0 until the first `UpdateTip()`.
Changed current_tip argument to `uint256` instead of `BlockRef`.
I also
...
👍 theStack approved a pull request: "validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2235126233)
Tested ACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410
Loaded the provided snapshot on a freshly created pruned node (`-prune=20000`), and waited first until the snapshot chainstate left IBD, then until the background validation was finished successfully. Right after loading, I also checked that UTXO-querying RPCs like `gettxoutsetinfo` / `scantxoutset` / `gettxout` use the chainstate snapshot (see also PR #30636). Also created a dump on another node on height 840000 and verified that the Assume
...
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2235126233)
Tested ACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410
Loaded the provided snapshot on a freshly created pruned node (`-prune=20000`), and waited first until the snapshot chainstate left IBD, then until the background validation was finished successfully. Right after loading, I also checked that UTXO-querying RPCs like `gettxoutsetinfo` / `scantxoutset` / `gettxout` use the chainstate snapshot (see also PR #30636). Also created a dump on another node on height 840000 and verified that the Assume
...
📝 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.