🤔 TheCharlatan reviewed a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2449774883)
lgtm, but I don't think this should be prefixed with kernel: in both the title and the commit message. The point of having the notification interface is that the node code can extend kernel behaviour. The implementation details of that are not part of the kernel.
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2449774883)
lgtm, but I don't think this should be prefixed with kernel: in both the title and the commit message. The point of having the notification interface is that the node code can extend kernel behaviour. The implementation details of that are not part of the kernel.
💬 TheCharlatan commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851029691)
Can't you skip the first conditional too then?
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851029691)
Can't you skip the first conditional too then?
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851034724)
`IsArgSet` would be true when the user passes `-noversion`, so the `if`-block would be entered, which is intended to be avoided in this PR.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851034724)
`IsArgSet` would be true when the user passes `-noversion`, so the `if`-block would be entered, which is intended to be avoided in this PR.
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851054194)
I fail to see the typo, mind pointing it out?
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851054194)
I fail to see the typo, mind pointing it out?
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851065265)
Potentially yes, but I'm trying to keep the scope of the PR to the args I discovered here: https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013
Open to exploring it in a follow-up!
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851065265)
Potentially yes, but I'm trying to keep the scope of the PR to the args I discovered here: https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013
Open to exploring it in a follow-up!
💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2489670559)
> Can this use the same approach and error message as `combinepsbt`?
This is the same functional approach and error message already.
Creating a PSBT involves stripping the scriptSig and scriptWitnesses. Later, the hashes are compared. That's exactly what this change is doing too but the syntax is different. I can align the syntax to make it more clear it is doing the same thing.
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2489670559)
> Can this use the same approach and error message as `combinepsbt`?
This is the same functional approach and error message already.
Creating a PSBT involves stripping the scriptSig and scriptWitnesses. Later, the hashes are compared. That's exactly what this change is doing too but the syntax is different. I can align the syntax to make it more clear it is doing the same thing.
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851068380)
I prefer documenting the type.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851068380)
I prefer documenting the type.
💬 andremralves commented on issue "Functional tests: `feature_bind_port_discover.py` is failing":
(https://github.com/bitcoin/bitcoin/issues/31336#issuecomment-2489678021)
I followed the instructions in found here:
https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/test/functional/feature_bind_port_discover.py#L12-L21
After https://github.com/bitcoin/bitcoin/pull/29984 I think those instructions no longer apply.
(https://github.com/bitcoin/bitcoin/issues/31336#issuecomment-2489678021)
I followed the instructions in found here:
https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/test/functional/feature_bind_port_discover.py#L12-L21
After https://github.com/bitcoin/bitcoin/pull/29984 I think those instructions no longer apply.
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851080067)
You mean
```
LogInfo("Using random cookie authentication.\n");
```
was run independently of whether `GenerateAuthCookie` returned `true` or `false`, but now it may not be? It seemed wrong to me so I made it only happen when the cookie was actually generated.
If you mean that
```
LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851080067)
You mean
```
LogInfo("Using random cookie authentication.\n");
```
was run independently of whether `GenerateAuthCookie` returned `true` or `false`, but now it may not be? It seemed wrong to me so I made it only happen when the cookie was actually generated.
If you mean that
```
LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth
...
💬 luke-jr commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2489692408)
>It currently doesn't give me any results for x9, IIRC the most common filter:
Same here
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2489692408)
>It currently doesn't give me any results for x9, IIRC the most common filter:
Same here
💬 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
...