💬 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.
🤔 mzumsande reviewed a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2862386239)
post-merge code review ACK fd290730f530a8b76a9607392f49830697cdd7c5
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2862386239)
post-merge code review ACK fd290730f530a8b76a9607392f49830697cdd7c5
💬 mzumsande commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103279774)
just noting that `CheckInputScripts()` will always return `true` in the multi-threaded case (it doesn't do any validation after all), so this logic could be simpler / `tx_ok` isn't really necessary.
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103279774)
just noting that `CheckInputScripts()` will always return `true` in the multi-threaded case (it doesn't do any validation after all), so this logic could be simpler / `tx_ok` isn't really necessary.
💬 fjahr commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103287538)
Hopefully this is soon obsolete with #32575 :)
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103287538)
Hopefully this is soon obsolete with #32575 :)
💬 ryanofsky commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2902382891)
Looking at 4837fdc1569d58541fb310251b969ae160907e6e:
- The change to subprocess.h seems like an improvement. Seems to make more sense to construct command line strings and pass them to the shell to parse, than construct command line strings and parse the strings we constructed ourselves.
- It would probably be good if the new behavior were controlled by a shell boolean option like python & the upstream library
- Windows seems to be doing joining, splitting, and the joining again, but that i
...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2902382891)
Looking at 4837fdc1569d58541fb310251b969ae160907e6e:
- The change to subprocess.h seems like an improvement. Seems to make more sense to construct command line strings and pass them to the shell to parse, than construct command line strings and parse the strings we constructed ourselves.
- It would probably be good if the new behavior were controlled by a shell boolean option like python & the upstream library
- Windows seems to be doing joining, splitting, and the joining again, but that i
...
🤔 sipa reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2862413711)
> In real-world scenarios it is unlikely we encounter those transactions that will exceed the cluster count limit, however when it occur I assume that the newly connected block will have similar transaction with the reorged block. I think maintaining an in-memory data structure for these transactions removed due to the limit seperate from mempool will reduce bandwidth from re-questing the tx again and also avoid non-contextual checks?
In theory, almost the entire mempool could be trimmed away
...
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2862413711)
> In real-world scenarios it is unlikely we encounter those transactions that will exceed the cluster count limit, however when it occur I assume that the newly connected block will have similar transaction with the reorged block. I think maintaining an in-memory data structure for these transactions removed due to the limit seperate from mempool will reduce bandwidth from re-questing the tx again and also avoid non-contextual checks?
In theory, almost the entire mempool could be trimmed away
...
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103301238)
To be clear, `Cluster::IsOversized()` returns whether the Cluster is oversized in general. It's just the case that changes to clusters which bring it over the size/count limit are never applied, so the only way an actually materialized Cluster can be oversized is if it's a singleton transaction which is oversized on its own.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103301238)
To be clear, `Cluster::IsOversized()` returns whether the Cluster is oversized in general. It's just the case that changes to clusters which bring it over the size/count limit are never applied, so the only way an actually materialized Cluster can be oversized is if it's a singleton transaction which is oversized on its own.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103305596)
You're right, this doesn't matter at all. The Cluster will just be deleted by `Split` anyway. Removed it.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103305596)
You're right, this doesn't matter at all. The Cluster will just be deleted by `Split` anyway. Removed it.