👍 lucasbalieiro approved a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2449947660)
**Hi @MarnixCroes!**
**tACK** [b3a96b3](https://github.com/bitcoin/bitcoin/pull/31332/commits/b3a96b35691f3fbf958958056cd905e119fb48fe)
### **What I Understand**
The incoming changes to the build process introduce `cmake`, which builds in the `./build` directory. This PR updates the `BUILDDIR` path to correctly point to the new `./build` directory generated by `cmake`. This update is proposed to ensure compatibility with in-tree builds, as described in `contrib/devtools/README.md`:
> W
...
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2449947660)
**Hi @MarnixCroes!**
**tACK** [b3a96b3](https://github.com/bitcoin/bitcoin/pull/31332/commits/b3a96b35691f3fbf958958056cd905e119fb48fe)
### **What I Understand**
The incoming changes to the build process introduce `cmake`, which builds in the `./build` directory. This PR updates the `BUILDDIR` path to correctly point to the new `./build` directory generated by `cmake`. This update is proposed to ensure compatibility with in-tree builds, as described in `contrib/devtools/README.md`:
> W
...
👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2450023917)
Code review ACK c305ad5b06545497752b2f12d8d9f72e8a6c2941. This is a nice change that should make the RPC (and test) more reliable, and the implementation seems to be pretty simple now.
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2450023917)
Code review ACK c305ad5b06545497752b2f12d8d9f72e8a6c2941. This is a nice change that should make the RPC (and test) more reliable, and the implementation seems to be pretty simple now.
💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851184397)
In commit "rpc: add optional blockhash to waitfornewblock" (c305ad5b06545497752b2f12d8d9f72e8a6c2941)
Might be useful if comment explained the different cases here. Maybe: // If caller provided a current_tip value, pass it to waitTipChanged. If the caller did not provide a current tip hash, call getTip() to get one and wait for the tip to be different from this value. (This mode is less reliable because if the tip changed between waitfornewblock calls, it will need to change a second time bef
...
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851184397)
In commit "rpc: add optional blockhash to waitfornewblock" (c305ad5b06545497752b2f12d8d9f72e8a6c2941)
Might be useful if comment explained the different cases here. Maybe: // If caller provided a current_tip value, pass it to waitTipChanged. If the caller did not provide a current tip hash, call getTip() to get one and wait for the tip to be different from this value. (This mode is less reliable because if the tip changed between waitfornewblock calls, it will need to change a second time bef
...
💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851191687)
In commit "rpc: add optional blockhash to waitfornewblock" (c305ad5b06545497752b2f12d8d9f72e8a6c2941)
Note: dropping IsRPCRunning call is a minor change in behavior, but probably a good one, as it should let the waitfornewblock function in the GUI console even if the RPC server is not enabled. In any case it is safe to drop this because waitTipChanged already will return if the node is shutting down.
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851191687)
In commit "rpc: add optional blockhash to waitfornewblock" (c305ad5b06545497752b2f12d8d9f72e8a6c2941)
Note: dropping IsRPCRunning call is a minor change in behavior, but probably a good one, as it should let the waitfornewblock function in the GUI console even if the RPC server is not enabled. In any case it is safe to drop this because waitTipChanged already will return if the node is shutting down.
💬 kevkevinpal commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#issuecomment-2489923051)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31323#issuecomment-2489923051)
Concept ACK
👍 ryanofsky approved a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2450052356)
Code review ACK b283bc8dae0e0b714bdf1828579c67ab94da2cc7.
Can ignore this, but I want to express a slightly dissenting opinion on this change. I don't object to it but wouldn't have recommended it, because checking for `hash.IsNull()` is so widespread in the code, and I think writing interoperable code becomes more error prone when there are multiple ways of describing an unset hash instead of standardizing on one. But on the other hand I understand using `std::optional` can be more self-docu
...
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2450052356)
Code review ACK b283bc8dae0e0b714bdf1828579c67ab94da2cc7.
Can ignore this, but I want to express a slightly dissenting opinion on this change. I don't object to it but wouldn't have recommended it, because checking for `hash.IsNull()` is so widespread in the code, and I think writing interoperable code becomes more error prone when there are multiple ways of describing an unset hash instead of standardizing on one. But on the other hand I understand using `std::optional` can be more self-docu
...
💬 ryanofsky commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851203430)
re: https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750
> Can't you skip the first conditional too then?
We do need the first condition because we want this to wait for m_tip_block to be set and for the value to be different than the current_tip value. Without the first condition this wouldn't wait at all for m_tip_block to be set.
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851203430)
re: https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750
> Can't you skip the first conditional too then?
We do need the first condition because we want this to wait for m_tip_block to be set and for the value to be different than the current_tip value. Without the first condition this wouldn't wait at all for m_tip_block to be set.
💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2489948972)
Ready for a re-review when you're ready, @tdb3 @rkrux.
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2489948972)
Ready for a re-review when you're ready, @tdb3 @rkrux.
👍 ryanofsky approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2450106183)
Code review ACK 1dfc54d87c7b35cc93cfb81cc2a46c9e8eae7194. Left a style suggestion, and a suggestion to split up the first commit, if we want to do that. But these can be ignored and this all looks good as-is. Makes a lot of sense to not make a parameter which should only be used for testing a required argument.
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2450106183)
Code review ACK 1dfc54d87c7b35cc93cfb81cc2a46c9e8eae7194. Left a style suggestion, and a suggestion to split up the first commit, if we want to do that. But these can be ignored and this all looks good as-is. Makes a lot of sense to not make a parameter which should only be used for testing a required argument.
💬 ryanofsky commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851245779)
In commit "Drop script_pub_key arg from createNewBlock" (42f880aeb080070a42229d0bccd517100a3fe7fb)
Would it make sense to change the `BlockAssembler::CreateNewBlock()` signature in a separate followup commit? The test changes in this commit vastly outnumber the non-test changes, which I think make it more likely a bug in the non-test code could be missed.
Maybe this there could be two overloads initially:
```c++
std::unique_ptr<CBlockTemplate> CreateNewBlock()
{
return CreateNewB
...
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851245779)
In commit "Drop script_pub_key arg from createNewBlock" (42f880aeb080070a42229d0bccd517100a3fe7fb)
Would it make sense to change the `BlockAssembler::CreateNewBlock()` signature in a separate followup commit? The test changes in this commit vastly outnumber the non-test changes, which I think make it more likely a bug in the non-test code could be missed.
Maybe this there could be two overloads initially:
```c++
std::unique_ptr<CBlockTemplate> CreateNewBlock()
{
return CreateNewB
...
💬 ryanofsky commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851238482)
In commit "Drop script_pub_key arg from createNewBlock" (42f880aeb080070a42229d0bccd517100a3fe7fb)
Would suggest using '*' instead of `.value()` since this code isn't intending to trigger the `std::bad_optional_access` exception, and to make it more obvious a dereference is happening.
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851238482)
In commit "Drop script_pub_key arg from createNewBlock" (42f880aeb080070a42229d0bccd517100a3fe7fb)
Would suggest using '*' instead of `.value()` since this code isn't intending to trigger the `std::bad_optional_access` exception, and to make it more obvious a dereference is happening.
👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177)
Code review and test ACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2
Really liking how this turned out.
Left a couple of minor things. None blocking.
<details>
<summary>quick retest with tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg</summary>
```
$ build/src/bitcoin-cli scanblocks start '["addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)"]'
{
"from_height": 0,
"to_height": 222935,
"relevant_blocks": [
"000000ddbaa40d82c7ae3e3188347dd77858f37e57aae0e51d004
...
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177)
Code review and test ACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2
Really liking how this turned out.
Left a couple of minor things. None blocking.
<details>
<summary>quick retest with tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg</summary>
```
$ build/src/bitcoin-cli scanblocks start '["addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)"]'
{
"from_height": 0,
"to_height": 222935,
"relevant_blocks": [
"000000ddbaa40d82c7ae3e3188347dd77858f37e57aae0e51d004
...
💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851274367)
non-blocking nit
If needing to touch this file again:
> If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts
https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851274367)
non-blocking nit
If needing to touch this file again:
> If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts
https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851301558)
For a follow-up (or if touching this file again before merge):
Could explicitly use `getnewdestination(address_type='bech32m')` instead of relying on the default.
Later in the function, the `output_spk` of the returned activity assumes p2tr. This currently works because bech32m is the default `address_type` argument of `getnewdestination()`. Being explicit prevents future changes from causing this test to fail.
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851301558)
For a follow-up (or if touching this file again before merge):
Could explicitly use `getnewdestination(address_type='bech32m')` instead of relying on the default.
Later in the function, the `output_spk` of the returned activity assumes p2tr. This currently works because bech32m is the default `address_type` argument of `getnewdestination()`. Being explicit prevents future changes from causing this test to fail.
💬 tdb3 commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2490065953)
Thanks. Planning to touch this up soon.
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2490065953)
Thanks. Planning to touch this up soon.
💬 naumenkogs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2490182594)
ACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12
a trivial refactoring since the last ack
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2490182594)
ACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12
a trivial refactoring since the last ack
💬 1440000bytes commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2490302102)
> Later, the hashes are compared. That's exactly what this change is doing too but the syntax is different.
```c++
bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
{
// Prohibited to merge two PSBTs over different transactions
if (tx->GetHash() != psbt.tx->GetHash()) {
return false;
}
```
Right. Only the error message was different which looks same now,
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2490302102)
> Later, the hashes are compared. That's exactly what this change is doing too but the syntax is different.
```c++
bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
{
// Prohibited to merge two PSBTs over different transactions
if (tx->GetHash() != psbt.tx->GetHash()) {
return false;
}
```
Right. Only the error message was different which looks same now,
👍 1440000bytes approved a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298#pullrequestreview-2450523630)
utACK https://github.com/bitcoin/bitcoin/pull/31298/commits/85c216871a01aab003e1b1aba6246b4178afc929
(https://github.com/bitcoin/bitcoin/pull/31298#pullrequestreview-2450523630)
utACK https://github.com/bitcoin/bitcoin/pull/31298/commits/85c216871a01aab003e1b1aba6246b4178afc929
🤔 maflcko reviewed a pull request: "build: Use `-ffile-prefix-map` in release builds only"
(https://github.com/bitcoin/bitcoin/pull/31337#pullrequestreview-2450548902)
Good catch, but I think the upstream docs were wrong in this case. For me, they don't mention coverage is affected as well: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffile-prefix-map
(https://github.com/bitcoin/bitcoin/pull/31337#pullrequestreview-2450548902)
Good catch, but I think the upstream docs were wrong in this case. For me, they don't mention coverage is affected as well: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffile-prefix-map
💬 maflcko commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1851542525)
Why is this needed? guix never compiles with `--coverage`, no? See https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fprofile-prefix-map
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1851542525)
Why is this needed? guix never compiles with `--coverage`, no? See https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fprofile-prefix-map