💬 instagibbs commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103092771)
we don't know they were missing them really, just mention they asked for them for the specific block?
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103092771)
we don't know they were missing them really, just mention they asked for them for the specific block?
🤔 ryanofsky reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2859167293)
Great review! I improved a number of things in response to your comments.
Rebased 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa -> 6bf727fb88009f99dab7c06bff7f8fb391147ac1 ([`pr/ipc-cli.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.5) -> [`pr/ipc-cli.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.5-rebase..pr/ipc-cli.6)) due to conflict with #28710, also made a number of improvements to -ipcbind docume
...
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2859167293)
Great review! I improved a number of things in response to your comments.
Rebased 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa -> 6bf727fb88009f99dab7c06bff7f8fb391147ac1 ([`pr/ipc-cli.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.5) -> [`pr/ipc-cli.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.5-rebase..pr/ipc-cli.6)) due to conflict with #28710, also made a number of improvements to -ipcbind docume
...
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101199458)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693
> I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.
Makes sense, updated ipcbind help text to mention RPC authentication.
> You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.
...
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101199458)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693
> I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.
Makes sense, updated ipcbind help text to mention RPC authentication.
> You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.
...
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101228151)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369
> Might want to indicate `status` is an "out" param?
Thanks, expanded comment.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101228151)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369
> Might want to indicate `status` is an "out" param?
Thanks, expanded comment.
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101223433)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845
> Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`
Nice catch, dropped the unused return value.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101223433)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845
> Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`
Nice catch, dropped the unused return value.
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101237982)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092
> I presume you can ignore `rpcwait` because the socket won't exist until the server is running?
Not really, this is a good catch. I was just ignoring rpcwait option before but updated the code now so it will work for both RPC and IPC. Now the connection failed case is handled the same for both.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101237982)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092
> I presume you can ignore `rpcwait` because the socket won't exist until the server is running?
Not really, this is a good catch. I was just ignoring rpcwait option before but updated the code now so it will work for both RPC and IPC. Now the connection failed case is handled the same for both.
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101257896)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905
> Hm for some reason I thought it could be as low as 92:
You are right. I added some code to fall back to 92 if this is not a known platform with a higher limit. This could use 92 everywhere, but since test coverage here is lost when the datadir path length exceeds this value, I think it is good to run the extra checks more places if the platform does support a higher limit.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101257896)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905
> Hm for some reason I thought it could be as low as 92:
You are right. I added some code to fall back to 92 if this is not a known platform with a higher limit. This could use 92 everywhere, but since test coverage here is lost when the datadir path length exceeds this value, I think it is good to run the extra checks more places if the platform does support a higher limit.
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2103137795)
It is used for multipath descriptors, tests of which are added 2 commits later.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2103137795)
It is used for multipath descriptors, tests of which are added 2 commits later.
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2103139861)
I find that harder to read. `for i` is used here because we need the position.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2103139861)
I find that harder to read. `for i` is used here because we need the position.
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2103140839)
In the next commit which implements parsing.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2103140839)
In the next commit which implements parsing.
📝 theuni opened a pull request: "threading: remove ancient CRITICAL_SECTION macros"
(https://github.com/bitcoin/bitcoin/pull/32592)
Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.
This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.
While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang
...
(https://github.com/bitcoin/bitcoin/pull/32592)
Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.
This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.
While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang
...
💬 theuni commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2902192274)
Ready for rebase.
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2902192274)
Ready for rebase.
💬 achow101 commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2103202649)
I actually did try to implement it with an enum originally, but it got pretty complicated and I think just having it has a bool is significantly easier to read and reason about.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2103202649)
I actually did try to implement it with an enum originally, but it got pretty complicated and I think just having it has a bool is significantly easier to read and reason about.
💬 achow101 commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2103207660)
> Would removing this property from the response categorise the diff as a breaking change?
Yes, that's mainly why this is preserved.
> Looks a little odd to me to display this as true for watch only wallets as well because the wallet doesn't have private keys and thus I feel it should not be a decision maker on this.
This is the current behavior anyways.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2103207660)
> Would removing this property from the response categorise the diff as a breaking change?
Yes, that's mainly why this is preserved.
> Looks a little odd to me to display this as true for watch only wallets as well because the wallet doesn't have private keys and thus I feel it should not be a decision maker on this.
This is the current behavior anyways.
💬 achow101 commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2103209325)
Done
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2103209325)
Done
👍 pinheadmz approved a pull request: "rpc: Round verificationprogress to 1 for a recent tip"
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2862281109)
ACK fa53098472521de9d5b3cc42983043c240b7c321
Tested and reviewed code. Might warrant a release note: "Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%".
In addition to this UI change, block tip signal callers now compute the estimated progress as opposed to the signal handlers, and `CBlockIndex` are being passed by reference instead of raw pointer through the signal-hand
...
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2862281109)
ACK fa53098472521de9d5b3cc42983043c240b7c321
Tested and reviewed code. Might warrant a release note: "Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%".
In addition to this UI change, block tip signal callers now compute the estimated progress as opposed to the signal handlers, and `CBlockIndex` are being passed by reference instead of raw pointer through the signal-hand
...
💬 ryanofsky commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2902300014)
Concept ACK. Nice idea, and it does seem useful to have a macro checking for unexpected but not very serious conditions by throwing an exception that gets reported in release builds but is a fatal error in debug builds. And current uses of the macro seem like good candidates for that behavior.
The only possible issues I see are:
(1) The name CHECK_NONFATAL doesn't make a lot of sense anymore, now triggering fatal errors when it literally says "nonfatal" in the name.
(2) It is now more cum
...
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2902300014)
Concept ACK. Nice idea, and it does seem useful to have a macro checking for unexpected but not very serious conditions by throwing an exception that gets reported in release builds but is a fatal error in debug builds. And current uses of the macro seem like good candidates for that behavior.
The only possible issues I see are:
(1) The name CHECK_NONFATAL doesn't make a lot of sense anymore, now triggering fatal errors when it literally says "nonfatal" in the name.
(2) It is now more cum
...
💬 theuni commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2902311325)
As mentioned in the description, failing tests are just demonstrating the borked REVERSE_LOCK fixed by #32465.
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2902311325)
As mentioned in the description, failing tests are just demonstrating the borked REVERSE_LOCK fixed by #32465.
💬 theuni commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2902350217)
I'm also not in favor of this, but now it's because I think this is addressing the wrong issue altogether.
Generally I think this is a nice change, but it's scary, and if we're going to be changing it I'd rather do it once and for all. As part of the `CConnman` refactor, `CNode` was supposed to become internal and not passed into `PeerManager` at all. In that case, the lifetime and sharing issues would be completely moot. But that didn't get done.. my fault.
So, I'm working on doing that n
...
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2902350217)
I'm also not in favor of this, but now it's because I think this is addressing the wrong issue altogether.
Generally I think this is a nice change, but it's scary, and if we're going to be changing it I'd rather do it once and for all. As part of the `CConnman` refactor, `CNode` was supposed to become internal and not passed into `PeerManager` at all. In that case, the lifetime and sharing issues would be completely moot. But that didn't get done.. my fault.
So, I'm working on doing that n
...
💬 douglaz commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2902357150)
> We’ve already seen a clear signal that people don’t want this, with a noticeable migration from Bitcoin Core to Bitcoin Knots over the past three weeks.
It’s clear we’ve agreed to disagree: people who prefer arbitrary filters will run Bitcoin Knots, and people who prefer harmless, consensus-valid transactions will run Bitcoin Core.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2902357150)
> We’ve already seen a clear signal that people don’t want this, with a noticeable migration from Bitcoin Core to Bitcoin Knots over the past three weeks.
It’s clear we’ve agreed to disagree: people who prefer arbitrary filters will run Bitcoin Knots, and people who prefer harmless, consensus-valid transactions will run Bitcoin Core.