💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851090795)
Yeah, was itching to do that too, thanks for a second vote. :)
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851090795)
Yeah, was itching to do that too, thanks for a second vote. :)
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093183)
I want it to be easy for reviewers to drop the impl commits or re-order and run what is in the test commits to verify that I'm actually fixing issues.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093183)
I want it to be easy for reviewers to drop the impl commits or re-order and run what is in the test commits to verify that I'm actually fixing issues.
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093620)
https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093183
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093620)
https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093183
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851098868)
Good point about double negation. Changed to:
```C++
// If the file is explicitly specified, it must be readable
```
since the distinction is important.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851098868)
Good point about double negation. Changed to:
```C++
// If the file is explicitly specified, it must be readable
```
since the distinction is important.
👍 hodlinator approved a pull request: "macOS: swap docs & CI from pkg-config to pkgconf"
(https://github.com/bitcoin/bitcoin/pull/31335#pullrequestreview-2449903968)
utACK fa9e7fb167d7a1c476af931610ccbae39e29b964
Don't have a valid Mac to test on but change looks very straightforward and is in a similar vane to the linked upstream change.
(https://github.com/bitcoin/bitcoin/pull/31335#pullrequestreview-2449903968)
utACK fa9e7fb167d7a1c476af931610ccbae39e29b964
Don't have a valid Mac to test on but change looks very straightforward and is in a similar vane to the linked upstream change.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1851112761)
I was able to contrive a test that requires this loop, which also revealed a bug.
The situation is if a `sh(wsh(pkh()))` is imported with `importmulti`, the `wsh(pkh())` will only be in `mapScripts`. However, this `wsh(pkh())` is still `ISMINE_SPENDABLE` and coins sent to it will be seen by the wallet. This loop is where that would be detected.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1851112761)
I was able to contrive a test that requires this loop, which also revealed a bug.
The situation is if a `sh(wsh(pkh()))` is imported with `importmulti`, the `wsh(pkh())` will only be in `mapScripts`. However, this `wsh(pkh())` is still `ISMINE_SPENDABLE` and coins sent to it will be seen by the wallet. This loop is where that would be detected.
👍 tdb3 approved a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333#pullrequestreview-2449906384)
ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
Lightly tested by running with `FUZZ=utxo_snapshot`.
Verified the observation in the description `<name>/<random>`.
(https://github.com/bitcoin/bitcoin/pull/31333#pullrequestreview-2449906384)
ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
Lightly tested by running with `FUZZ=utxo_snapshot`.
Verified the observation in the description `<name>/<random>`.
💬 hodlinator commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2489745328)
> Add a workaround for #31334.
Sounds like a TODO? Why not "Fixes #31334"?
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2489745328)
> Add a workaround for #31334.
Sounds like a TODO? Why not "Fixes #31334"?
💬 luke-jr commented on pull request "rpc: print P2WSH witScript in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r1851133029)
This isn't an indicator of p2wsh... Maybe use d1f0ff0e1fed2c4124d0d81f18daf7b76a770dda or similar?
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r1851133029)
This isn't an indicator of p2wsh... Maybe use d1f0ff0e1fed2c4124d0d81f18daf7b76a770dda or similar?
💬 lucasbalieiro commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script":
(https://github.com/bitcoin/bitcoin/pull/31332#issuecomment-2489778682)
**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#issuecomment-2489778682)
**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
...
👍 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
...