💬 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
...
👍 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
...