🤔 janb84 reviewed a pull request: "scripted-diff: fix leftover references to `policy/fees.h`"
(https://github.com/bitcoin/bitcoin/pull/33864#pullrequestreview-3455154399)
ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
Trivial cleanup PR.
This time I did some extra code searches to find any possible leftovers, could not find one.
(https://github.com/bitcoin/bitcoin/pull/33864#pullrequestreview-3455154399)
ACK b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5
Trivial cleanup PR.
This time I did some extra code searches to find any possible leftovers, could not find one.
💬 janb84 commented on pull request "ci: Enable experimental kernel stuff in most CI tasks":
(https://github.com/bitcoin/bitcoin/pull/33824#issuecomment-3523635392)
Since the "side-effect" of this pr will enable the kernel stuff is the PR title still fitting ?
Maybe something like: CI: base taks on `dev-mode`
(https://github.com/bitcoin/bitcoin/pull/33824#issuecomment-3523635392)
Since the "side-effect" of this pr will enable the kernel stuff is the PR title still fitting ?
Maybe something like: CI: base taks on `dev-mode`
💬 hebasto commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#issuecomment-3523732124)
Friendly ping @purpleKarrot :)
(https://github.com/bitcoin/bitcoin/pull/33865#issuecomment-3523732124)
Friendly ping @purpleKarrot :)
💬 vicjuma commented on pull request "scripted-diff: fix leftover references to `policy/fees.h`":
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3523754781)
ACK the OP’s latest commit.
Confirmed that `git grep "policy/fees.h"` does not return any lines, meaning that the solution works.
> git grep -l "policy\/fees\.h" | xargs sed -i "s/policy\/fees.h/policy\/fees\/block_policy_estimator.h/g"
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3523754781)
ACK the OP’s latest commit.
Confirmed that `git grep "policy/fees.h"` does not return any lines, meaning that the solution works.
> git grep -l "policy\/fees\.h" | xargs sed -i "s/policy\/fees.h/policy\/fees\/block_policy_estimator.h/g"
💬 TheCharlatan commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2519703206)
How about keeping the checking, but testing it without going through the c++ wrapper?
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2519703206)
How about keeping the checking, but testing it without going through the c++ wrapper?
💬 purpleKarrot commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2519712187)
I think the `_$<CONFIG>` part is unnecessary, because the value of the `LOCATION` property is already config dependent.
`$<PATH:GET_*,...>` requires CMake version 3.24.
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2519712187)
I think the `_$<CONFIG>` part is unnecessary, because the value of the `LOCATION` property is already config dependent.
`$<PATH:GET_*,...>` requires CMake version 3.24.
🤔 hebasto reviewed a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455373185)
Tested `bitcoin-qt` from `bitcoin-79d5f120f4d1-x86_64-linux-gnu.tar.gz` on Fedora 42. It works without issues. `QXcbIntegrationPlugin` loaded correctly:
```
2025-11-12T20:21:43Z Bitcoin Core version v30.99.0-g79d5f120f4d10b681d3cebad1fcabff3c6f0c8fc (release build)
2025-11-12T20:21:43Z Qt 6.7.3 (static), plugin=xcb
2025-11-12T20:21:43Z Static plugins:
2025-11-12T20:21:43Z QMinimalIntegrationPlugin, version 395008
2025-11-12T20:21:43Z QXcbIntegrationPlugin, version 395008
2025-11-12T20:21:43Z S
...
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455373185)
Tested `bitcoin-qt` from `bitcoin-79d5f120f4d1-x86_64-linux-gnu.tar.gz` on Fedora 42. It works without issues. `QXcbIntegrationPlugin` loaded correctly:
```
2025-11-12T20:21:43Z Bitcoin Core version v30.99.0-g79d5f120f4d10b681d3cebad1fcabff3c6f0c8fc (release build)
2025-11-12T20:21:43Z Qt 6.7.3 (static), plugin=xcb
2025-11-12T20:21:43Z Static plugins:
2025-11-12T20:21:43Z QMinimalIntegrationPlugin, version 395008
2025-11-12T20:21:43Z QXcbIntegrationPlugin, version 395008
2025-11-12T20:21:43Z S
...
💬 hebasto commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2519797731)
```suggestion
@@ -504,6 +504,7 @@ qt_config_compile_test(xcb_syslibs
```
to avoid
```
patching file qtbase/src/gui/configure.cmake
Hunk #1 succeeded at 504 (offset 5 lines).
```
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2519797731)
```suggestion
@@ -504,6 +504,7 @@ qt_config_compile_test(xcb_syslibs
```
to avoid
```
patching file qtbase/src/gui/configure.cmake
Hunk #1 succeeded at 504 (offset 5 lines).
```
💬 Crypt-iQ commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2519814656)
Is it always possible to split a 8 MWU template into two <= 4MB responses? Consider a (very) contrived example: a node for some reason has three non-standard jumbo taproot-spend txns in their mempool of sizes 2 MWU, 3 MWU, 3 MWU, and they go to generate a template for this. If the receiver doesn't have these txns, there's no way they can fit in the max message size. At least, this is what I gather is possible from reading BIP 342.
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2519814656)
Is it always possible to split a 8 MWU template into two <= 4MB responses? Consider a (very) contrived example: a node for some reason has three non-standard jumbo taproot-spend txns in their mempool of sizes 2 MWU, 3 MWU, 3 MWU, and they go to generate a template for this. If the receiver doesn't have these txns, there's no way they can fit in the max message size. At least, this is what I gather is possible from reading BIP 342.
📝 TheCharlatan opened a pull request: "refactor: Let CCoinsViewCache::BatchWrite return void"
(https://github.com/bitcoin/bitcoin/pull/33866)
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Special case the coins view fuzz test since we now surface an exception instead of returning `false`, as previously implemented in `CCoinsView::BatchWrite`, when operating under a raw `CCoinsView`. This path should not be triggered in p
...
(https://github.com/bitcoin/bitcoin/pull/33866)
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Special case the coins view fuzz test since we now surface an exception instead of returning `false`, as previously implemented in `CCoinsView::BatchWrite`, when operating under a raw `CCoinsView`. This path should not be triggered in p
...
💬 plebhash commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3524052025)
> Ok, that's not really how you're supposed to use it :-)
hmm I guess this would indeed be a misuse of the interface if my end goal was to get Coinbase Tx data, and nothing more.
however, we **always need** to call `getBlock` specifically because it contains the `header` + `txdata` of the template... and as a byproduct, we happen to also get the Coinbase Tx data, which saves us one extra call to `getCoinbase`/`getCoinbaseTx`
at some point we'll start exploring the so called "Coinbase-only Sv2
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3524052025)
> Ok, that's not really how you're supposed to use it :-)
hmm I guess this would indeed be a misuse of the interface if my end goal was to get Coinbase Tx data, and nothing more.
however, we **always need** to call `getBlock` specifically because it contains the `header` + `txdata` of the template... and as a byproduct, we happen to also get the Coinbase Tx data, which saves us one extra call to `getCoinbase`/`getCoinbaseTx`
at some point we'll start exploring the so called "Coinbase-only Sv2
...
👍 ryanofsky approved a pull request: "mining: add getCoinbase()"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3454679064)
Code review ACK 80acabaf9353245f0839b18d9552ab39f595eda6. I left various suggestions below but none are critical. This seems like a thoughtful change that should make clients easier to write and be compatible with potential soft forks, without placing a lot of unnecessary requirements on them.
---
re: https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3520618909
> However there's another gotcha if we remove a method from the interface: there's no gaps allowed in the .capnp method numb
...
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3454679064)
Code review ACK 80acabaf9353245f0839b18d9552ab39f595eda6. I left various suggestions below but none are critical. This seems like a thoughtful change that should make clients easier to write and be compatible with potential soft forks, without placing a lot of unnecessary requirements on them.
---
re: https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3520618909
> However there's another gotcha if we remove a method from the interface: there's no gaps allowed in the .capnp method numb
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519494258)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Might be good to assert that vin[0] exists
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519494258)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Might be good to assert that vin[0] exists
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519522902)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
It seems like if this change could break clients, it might be better to return an error than a template that violates their expectations. Maybe would suggest:
```c++
if (!Assume(coinbase.script_sig_prefix.size() <= 8)) {
throw std::logic_error("Internal bug: coinbase data is inconsistent with CoinbaseTemplate definition. Data may be corrupt or definition may need to be updated");
}
```
Or could replac
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519522902)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
It seems like if this change could break clients, it might be better to return an error than a template that violates their expectations. Maybe would suggest:
```c++
if (!Assume(coinbase.script_sig_prefix.size() <= 8)) {
throw std::logic_error("Internal bug: coinbase data is inconsistent with CoinbaseTemplate definition. Data may be corrupt or definition may need to be updated");
}
```
Or could replac
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519478463)
re: https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518142243
> It turns out that using the `getCoinbase()` result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second `checkBlock()`.
>
> This probably happened once I extracted the coinbase specific checks into `build_coinbase_test`, which returns the coinbase `CTransaction` but presumably let go of the `getCoinbase()` result.
>
> Rather than debug (o
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519478463)
re: https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518142243
> It turns out that using the `getCoinbase()` result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second `checkBlock()`.
>
> This probably happened once I extracted the coinbase specific checks into `build_coinbase_test`, which returns the coinbase `CTransaction` but presumably let go of the `getCoinbase()` result.
>
> Rather than debug (o
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519524459)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Looks like || should be &&. Alternately could use two Assert statements.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519524459)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Looks like || should be &&. Alternately could use two Assert statements.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519564612)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
I think this comment might would be more helpful if moved to the `CoinbaseTemplate::output` field since it describes what that field contains. Also it should be visible there since that type is part of the external interface.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519564612)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
I think this comment might would be more helpful if moved to the `CoinbaseTemplate::output` field since it describes what that field contains. Also it should be visible there since that type is part of the external interface.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519536804)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Would seem good to assert `vout[0]` exists. Or maybe drop this line and unify with the loop below to make the relationship between `output` and `value_remaining` fields more direct.
```c++
for (const auto& output : coinbase_tx->vout) {
if (!output.scriptPubKey.empty() && output.scriptPubKey[0] == OP_RETURN) {
coinbase.outputs.push_back(output);
} else {
coinbase
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519536804)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Would seem good to assert `vout[0]` exists. Or maybe drop this line and unify with the loop below to make the relationship between `output` and `value_remaining` fields more direct.
```c++
for (const auto& output : coinbase_tx->vout) {
if (!output.scriptPubKey.empty() && output.scriptPubKey[0] == OP_RETURN) {
coinbase.outputs.push_back(output);
} else {
coinbase
...
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519741863)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Maybe s/coinbase/coinbase input/, would feel a little clearer
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519741863)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Maybe s/coinbase/coinbase input/, would feel a little clearer
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519774754)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Seems like it would be more consistent to either drop the `input_` prefix from this field or add it to `script_sig_prefix` field. I'd slightly prefer dropping the `input_` field so `CoinbaseTemplate` field names just try to describe what the fields mean, not how the transaction needs to be physically constructed.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519774754)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
Seems like it would be more consistent to either drop the `input_` prefix from this field or add it to `script_sig_prefix` field. I'd slightly prefer dropping the `input_` field so `CoinbaseTemplate` field names just try to describe what the fields mean, not how the transaction needs to be physically constructed.