✅ Sjors closed a pull request: "mining: add requestedOutputs field, e.g. for merged mining"
(https://github.com/bitcoin/bitcoin/pull/33890)
(https://github.com/bitcoin/bitcoin/pull/33890)
💬 Sjors commented on pull request "mining: add requestedOutputs field, e.g. for merged mining":
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3559527326)
Going to close this because I don't think it's the right approach.
We want to keep templates as reusable as possible in order to support multiple clients (potentially even a public service, though the node would not be doing this directly).
With that in mind it seems better if clients take the template and then customize it by adding their own outputs, rather than us customising it for them.
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3559527326)
Going to close this because I don't think it's the right approach.
We want to keep templates as reusable as possible in order to support multiple clients (potentially even a public service, though the node would not be doing this directly).
With that in mind it seems better if clients take the template and then customize it by adding their own outputs, rather than us customising it for them.
💬 w0xlt commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2547248797)
nit: Maybe the variables could be renamed in this PR for better clarity.
The “description” rows below can be converted into comments to document the variables.
| Current name | Suggestion | Description |
|----------|-------------|------|
|`state`|`accept_state`|determines if the block data is valid enough to be written to the disk and entered into the block index.|
|`bg_state`| `activation_state` |attempts to connect the block to the tip of the active chain (executing scripts and updatin
...
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2547248797)
nit: Maybe the variables could be renamed in this PR for better clarity.
The “description” rows below can be converted into comments to document the variables.
| Current name | Suggestion | Description |
|----------|-------------|------|
|`state`|`accept_state`|determines if the block data is valid enough to be written to the disk and entered into the block index.|
|`bg_state`| `activation_state` |attempts to connect the block to the tip of the active chain (executing scripts and updatin
...
💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256483)
> reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.
Added a commit for this to https://github.com/bitcoin/bitcoin/pull/33903, as that CI pull didn't have any review yet anyway.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256483)
> reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.
Added a commit for this to https://github.com/bitcoin/bitcoin/pull/33903, as that CI pull didn't have any review yet anyway.
💬 hodlinator commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256541)
Yes, I've seen similar segfaults when building for s390x on NixOS. Don't know if some distros apply custom patches or use other versions, or if they set up the environment differently in some other way.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256541)
Yes, I've seen similar segfaults when building for s390x on NixOS. Don't know if some distros apply custom patches or use other versions, or if they set up the environment differently in some other way.
💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547272545)
> Is this guaranteed to iterate over all members of vtx_missing[]?
Yes, because of the check below that:
https://github.com/bitcoin/bitcoin/blob/6b2d17b13220ea69c33b8cab41fe059ff758874c/src/blockencodings.cpp#L211-L212
We increment `tx_missing_offset` every time we take a transaction from `vtx_missing`, so the check enforces that we followed the `if` branch / took from `vtx_missing` just as many times as there are members in `vtx_missing`.
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547272545)
> Is this guaranteed to iterate over all members of vtx_missing[]?
Yes, because of the check below that:
https://github.com/bitcoin/bitcoin/blob/6b2d17b13220ea69c33b8cab41fe059ff758874c/src/blockencodings.cpp#L211-L212
We increment `tx_missing_offset` every time we take a transaction from `vtx_missing`, so the check enforces that we followed the `if` branch / took from `vtx_missing` just as many times as there are members in `vtx_missing`.
💬 maflcko commented on pull request "clang-format: Set Bitcoin Core IncludeCategories":
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547289151)
I think this is just the common approach to go from local to global, and also used in this codebase historically. This means:
* Priority -1 for the build-config (not sure why this is special). Changing it can be done in a different pull. Here I just want to document the existing order.
* Priority 1 for Bitcoin Core stuff
* Priority 2 for third-party libs (boost, QT)
* Priority 3 for the c++ stdlib (includes without a dot in them)
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547289151)
I think this is just the common approach to go from local to global, and also used in this codebase historically. This means:
* Priority -1 for the build-config (not sure why this is special). Changing it can be done in a different pull. Here I just want to document the existing order.
* Priority 1 for Bitcoin Core stuff
* Priority 2 for third-party libs (boost, QT)
* Priority 3 for the c++ stdlib (includes without a dot in them)
💬 davidgumberg commented on pull request "test: Retry download in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/33915#issuecomment-3559587991)
ACK https://github.com/bitcoin/bitcoin/pull/33915/commits/fad06f3bb436a97683e8248bfda1bd0d9998c899
Nice! I've also had issues with this locally, retrying once seems a good idea.
(https://github.com/bitcoin/bitcoin/pull/33915#issuecomment-3559587991)
ACK https://github.com/bitcoin/bitcoin/pull/33915/commits/fad06f3bb436a97683e8248bfda1bd0d9998c899
Nice! I've also had issues with this locally, retrying once seems a good idea.
💬 maflcko commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3559605455)
> My preference would be (1), but limit documentation / support to the most recent macOS version. It can be limited to the minimum tooling required to briefly run fuzzers.
Sounds good. So I guess this is up-for-grabs, and a dev with the latest macOS can just update the docs (https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer) with the steps that work with that version?
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3559605455)
> My preference would be (1), but limit documentation / support to the most recent macOS version. It can be limited to the minimum tooling required to briefly run fuzzers.
Sounds good. So I guess this is up-for-grabs, and a dev with the latest macOS can just update the docs (https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer) with the steps that work with that version?
💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-3559611938)
Just added https://github.com/bitcoin/bitcoin/pull/33916 to the review list. This PR adds a fuzz target for the `TransactionCanBeBumped` function from `feebumper`.
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-3559611938)
Just added https://github.com/bitcoin/bitcoin/pull/33916 to the review list. This PR adds a fuzz target for the `TransactionCanBeBumped` function from `feebumper`.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-3559618090)
<!-- begin push-33 -->
Rebased 6bab3c091b6e9f8a90255066e7fb7f063eede5bc -> 8e4ceb66a97fa14afc932d12f944662638ed87af ([`pr/ipc-gui.32`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-gui.32) -> [`pr/ipc-gui.33`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-gui.33), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-gui.32-rebase..pr/ipc-gui.33))<!-- end --> on top of base PR
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-3559618090)
<!-- begin push-33 -->
Rebased 6bab3c091b6e9f8a90255066e7fb7f063eede5bc -> 8e4ceb66a97fa14afc932d12f944662638ed87af ([`pr/ipc-gui.32`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-gui.32) -> [`pr/ipc-gui.33`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-gui.33), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-gui.32-rebase..pr/ipc-gui.33))<!-- end --> on top of base PR
🤔 mzumsande reviewed a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3489487344)
> CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.
I don't agree with that. I don't see `bitcoin-cli` and `bitcoind` as completely se
...
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3489487344)
> CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.
I don't agree with that. I don't see `bitcoin-cli` and `bitcoind` as completely se
...
💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547342770)
nit:
```suggestion
for (const auto &tx : resp.txn) tx_requested_size += tx->ComputeTotalSize();
```
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547342770)
nit:
```suggestion
for (const auto &tx : resp.txn) tx_requested_size += tx->ComputeTotalSize();
```
💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3559651569)
@maflcko friendly review ping :)
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3559651569)
@maflcko friendly review ping :)
💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3559658957)
> I tried rebasing onto #33785 but it wouldn't fix the warning.
Checked this by:
```
₿ git pr 32740
Switched to branch 'pr/32740'
```
Rebasing onto the merge commit of #33785:
```
₿ git rebase HEAD~8 --onto 490cb056f651f4247731e39efab35ef8e8a59833
```
Adding back `const` in commit 9bfcde056df7698ddf7ce9d958d5a9d3f3d0b43a:
ARM 32 build and Linux->Windows cross builds succeed on CI with GNU (GCC) 13.3:
https://github.com/hodlinator/bitcoin/actions/runs/19529971162/job/55910436956#s
...
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3559658957)
> I tried rebasing onto #33785 but it wouldn't fix the warning.
Checked this by:
```
₿ git pr 32740
Switched to branch 'pr/32740'
```
Rebasing onto the merge commit of #33785:
```
₿ git rebase HEAD~8 --onto 490cb056f651f4247731e39efab35ef8e8a59833
```
Adding back `const` in commit 9bfcde056df7698ddf7ce9d958d5a9d3f3d0b43a:
ARM 32 build and Linux->Windows cross builds succeed on CI with GNU (GCC) 13.3:
https://github.com/hodlinator/bitcoin/actions/runs/19529971162/job/55910436956#s
...
💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2547377106)
(Maybe `ConsumeBit(LE|BE)` would be more appropriate than `DecodeBit(LE|BE)`).
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2547377106)
(Maybe `ConsumeBit(LE|BE)` would be more appropriate than `DecodeBit(LE|BE)`).
🤔 w0xlt reviewed a pull request: "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3489553822)
This PR eliminates the ambiguity of the boolean return value and improves the clarity of both the internal and kernel APIs.
ACK https://github.com/bitcoin/bitcoin/pull/33856/commits/1f589e1af162c2c2705f0404496c549785f2f545
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3489553822)
This PR eliminates the ambiguity of the boolean return value and improves the clarity of both the internal and kernel APIs.
ACK https://github.com/bitcoin/bitcoin/pull/33856/commits/1f589e1af162c2c2705f0404496c549785f2f545
💬 brunoerg commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#discussion_r2547388515)
nit:
```suggestion
std::vector<uint256> merkle_path = TransactionMerklePath(block, /*position=*/0);
```
(https://github.com/bitcoin/bitcoin/pull/33858#discussion_r2547388515)
nit:
```suggestion
std::vector<uint256> merkle_path = TransactionMerklePath(block, /*position=*/0);
```
💬 maflcko commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2547406121)
> Now that #33785 has been merged we hopefully can use `const& ... Assert(` in this PR as long as it's rebased.
The `-Wno-error=dangling-reference` was removed again in https://github.com/bitcoin/bitcoin/pull/33840/files
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2547406121)
> Now that #33785 has been merged we hopefully can use `const& ... Assert(` in this PR as long as it's rebased.
The `-Wno-error=dangling-reference` was removed again in https://github.com/bitcoin/bitcoin/pull/33840/files
👍 brunoerg approved a pull request: "test: add unit test coverage for the empty leaves path in MerkleComputation"
(https://github.com/bitcoin/bitcoin/pull/33858#pullrequestreview-3489581346)
code review ACK ffcae82a68104c1992964b26c592b62cbca391bf
It's just a simple test addition to exercise `MerkleComputation` with an empty vector of leaves, it's ok. Note that it just checks the path is empty which is fine since there is a proposal to remove the unused `pmutated` and `proot` (https://github.com/bitcoin/bitcoin/pull/33805).
(https://github.com/bitcoin/bitcoin/pull/33858#pullrequestreview-3489581346)
code review ACK ffcae82a68104c1992964b26c592b62cbca391bf
It's just a simple test addition to exercise `MerkleComputation` with an empty vector of leaves, it's ok. Note that it just checks the path is empty which is fine since there is a proposal to remove the unused `pmutated` and `proot` (https://github.com/bitcoin/bitcoin/pull/33805).