👍 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.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519746515)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
I'm a little confused by "may move the witness reserved value" in this comment. I would think a future soft fork could restrict this value but not move it anywhere. BIP 141 says the coinbase witness has to be a single 32-byte so it doesn't seem like there is more flexibility here.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2519746515)
In commit "mining: add getCoinbase()" (80acabaf9353245f0839b18d9552ab39f595eda6)
I'm a little confused by "may move the witness reserved value" in this comment. I would think a future soft fork could restrict this value but not move it anywhere. BIP 141 says the coinbase witness has to be a single 32-byte so it doesn't seem like there is more flexibility here.
🤔 w0xlt reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3455640771)
reACK https://github.com/bitcoin/bitcoin/pull/32606/commits/32bfd6136134e7194ea0b56ec08498f66829fc40
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3455640771)
reACK https://github.com/bitcoin/bitcoin/pull/32606/commits/32bfd6136134e7194ea0b56ec08498f66829fc40
💬 w0xlt commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2519911724)
Maybe add a comment for clarification?
```suggestion
// Accept unsolicited cmpctblock *only* from peers we selected as high-bandwidth
// "to" (hb_to). Peers that merely requested HB *from* us (hb_from) don't qualify.
// This matches BIP152's "unsolicited in high-bandwidth mode" intent.
if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
```
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2519911724)
Maybe add a comment for clarification?
```suggestion
// Accept unsolicited cmpctblock *only* from peers we selected as high-bandwidth
// "to" (hb_to). Peers that merely requested HB *from* us (hb_from) don't qualify.
// This matches BIP152's "unsolicited in high-bandwidth mode" intent.
if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
```
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2519946934)
please update the code comments as well
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2519946934)
please update the code comments as well
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#issuecomment-3524102355)
big concept ACK, some tests and comments likely still need updating, but can't wait for this to be finally gone, thanks for taking care of it.
For the record this is a continuation of https://github.com/bitcoin/bitcoin/pull/33042:
> Note
CCoinsView::BatchWrite (and transitively CCoinsViewCache::Flush & CCoinsViewCache::Sync) were intentionally not changed here. While all implementations return true, the base CCoinsView::BatchWrite returns false. Changing this would cause coins_view tests to
...
(https://github.com/bitcoin/bitcoin/pull/33866#issuecomment-3524102355)
big concept ACK, some tests and comments likely still need updating, but can't wait for this to be finally gone, thanks for taking care of it.
For the record this is a continuation of https://github.com/bitcoin/bitcoin/pull/33042:
> Note
CCoinsView::BatchWrite (and transitively CCoinsViewCache::Flush & CCoinsViewCache::Sync) were intentionally not changed here. While all implementations return true, the base CCoinsView::BatchWrite returns false. Changing this would cause coins_view tests to
...
💬 w0xlt commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3524119756)
Adding a new field in the result called `feerate_unit` (string) to specify the unit used (`sat/vB` or `BTC/kvB`) may address this issue.
The current approach looks good to me, though.
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3524119756)
Adding a new field in the result called `feerate_unit` (string) to specify the unit used (`sat/vB` or `BTC/kvB`) may address this issue.
The current approach looks good to me, though.
💬 hebasto commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2519975141)
> I think the `_$<CONFIG>` part is unnecessary, because the value of the `LOCATION` property is already config dependent.
Are you sure about that? I tested your suggestion, and it returns the path to the “Release” plugin when building with `--config Debug`.
> `$<PATH:GET_*,...>` requires CMake version 3.24.
I think it's safe, as the version of CMake shipped with Visual Studio was [updated to 3.24.1](https://learn.microsoft.com/en-us/cpp/overview/what-s-new-for-visual-cpp-in-visual-studi
...
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2519975141)
> I think the `_$<CONFIG>` part is unnecessary, because the value of the `LOCATION` property is already config dependent.
Are you sure about that? I tested your suggestion, and it returns the path to the “Release” plugin when building with `--config Debug`.
> `$<PATH:GET_*,...>` requires CMake version 3.24.
I think it's safe, as the version of CMake shipped with Visual Studio was [updated to 3.24.1](https://learn.microsoft.com/en-us/cpp/overview/what-s-new-for-visual-cpp-in-visual-studi
...
🤔 hebasto reviewed a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455760913)
My Guix build:
```
aarch64
eacb251187a66b61d420c134b48b4081cac704cfaef3965bc10f61f2dedbe9b1 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/SHA256SUMS.part
245e7592e13a90600f6ccff66442e98027d9f36b868942c88a117c05625e603e guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu-debug.tar.gz
083a468e350320244bfb83b46f2015d28a31ab69845fd5f605e31742c9665e27 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu.tar.gz
6031b50181edff
...
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455760913)
My Guix build:
```
aarch64
eacb251187a66b61d420c134b48b4081cac704cfaef3965bc10f61f2dedbe9b1 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/SHA256SUMS.part
245e7592e13a90600f6ccff66442e98027d9f36b868942c88a117c05625e603e guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu-debug.tar.gz
083a468e350320244bfb83b46f2015d28a31ab69845fd5f605e31742c9665e27 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu.tar.gz
6031b50181edff
...
👍 ryanofsky approved a pull request: "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC"
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-3455789823)
Code review ACK b03feedadb411e15967659be83940d73ce977848. Assuming #33819 goes ahead, there should be less need for this change from the mining interface perspective. But not adding OP_0 seems like more sensible default behavior and all the changes here seem nice to make the test code and BlockAssembler code more explicit and clear about what they are doing.
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-3455789823)
Code review ACK b03feedadb411e15967659be83940d73ce977848. Assuming #33819 goes ahead, there should be less need for this change from the mining interface perspective. But not adding OP_0 seems like more sensible default behavior and all the changes here seem nice to make the test code and BlockAssembler code more explicit and clear about what they are doing.
🤔 hebasto reviewed a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455802540)
nit: a typo in commit 9af749f4a455d4b64080e33c707204dee5369a2d message:
"qdusviewer" --> "qdbusviewer".
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3455802540)
nit: a typo in commit 9af749f4a455d4b64080e33c707204dee5369a2d message:
"qdusviewer" --> "qdbusviewer".
💬 Eunovo commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3524198301)
> Also, attempting to reindex while offline wouldn't work anymore if we are below minchainwork.
I don't think it is particularly important that reindex be able to complete offline. As @hodlinator pointed out:
> Not the cleanest perhaps. But we have 2 main usages of `-reindex` AFAIK:
>
> * Regular node-runners which run into some kind of disk corruption issue - they should typically have peers available to sync headers from.
> * Developers doing benchmarks/other work who either have suf
...
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3524198301)
> Also, attempting to reindex while offline wouldn't work anymore if we are below minchainwork.
I don't think it is particularly important that reindex be able to complete offline. As @hodlinator pointed out:
> Not the cleanest perhaps. But we have 2 main usages of `-reindex` AFAIK:
>
> * Regular node-runners which run into some kind of disk corruption issue - they should typically have peers available to sync headers from.
> * Developers doing benchmarks/other work who either have suf
...