💬 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.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103297850)
Done.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103297850)
Done.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103303619)
The `GetBlockBuilder` function already states "While the returned object exists, no mutators on the main graph are allowed.". I'm open to putting a comment to that effect on each of the mutators where it matters (incl. `Trim`), but I feel like it should be done consistently.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103303619)
The `GetBlockBuilder` function already states "While the returned object exists, no mutators on the main graph are allowed.". I'm open to putting a comment to that effect on each of the mutators where it matters (incl. `Trim`), but I feel like it should be done consistently.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103305067)
Interesting, done! I have renamed the function to `MatchesOversizedClusters` and added a comment to clarify what it does.
I've also simplified it to just `if (is_oversized != set.Overlaps(component)) return false;`.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103305067)
Interesting, done! I have renamed the function to `MatchesOversizedClusters` and added a comment to clarify what it does.
I've also simplified it to just `if (is_oversized != set.Overlaps(component)) return false;`.
💬 theuni 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_r2103327107)
@mzumsande See the (hidden) discussion here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199
This was mostly added to prevent future footguns if an early false return is ever added. But yeah, it's meaningless and confusing as-is, and prompted @fjahr's PR above :)
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2103327107)
@mzumsande See the (hidden) discussion here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199
This was mostly added to prevent future footguns if an early false return is ever added. But yeah, it's meaningless and confusing as-is, and prompted @fjahr's PR above :)
💬 fjahr commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2902456340)
> Ready for rebase.
Done, ready for review :)
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2902456340)
> Ready for rebase.
Done, ready for review :)
🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2862453832)
Thanks for the review! This could be ready to merge with another ack
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2862453832)
Thanks for the review! This could be ready to merge with another ack
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103335995)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101317124
> in commit [9c8c688](https://github.com/bitcoin/bitcoin/commit/9c8c68891b43053acfe7b8eb9d2e0d2bcfcb4e1e): nit: the directory list seems outdated
Thanks! I made this change locally so it will be included here if there is another update, or in one of the followups.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103335995)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101317124
> in commit [9c8c688](https://github.com/bitcoin/bitcoin/commit/9c8c68891b43053acfe7b8eb9d2e0d2bcfcb4e1e): nit: the directory list seems outdated
Thanks! I made this change locally so it will be included here if there is another update, or in one of the followups.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103341290)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101326882
Thanks, again it's important to call the non-throwing overload here, because any error should be handled by trying to execute relative to the original, not throwing an error if symlinks can't be resolved for whatever reason.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103341290)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101326882
Thanks, again it's important to call the non-throwing overload here, because any error should be handled by trying to execute relative to the original, not throwing an error if symlinks can't be resolved for whatever reason.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103331130)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101317124
> in commit [5076d20](https://github.com/bitcoin/bitcoin/commit/5076d20fdb70a4bfafc4bdfe8293e347cb6bfa78): nit: since the error code `ec` isn't used, could use the shorter one-parameter variant of `is_regular_file`
The reason for doing this is the `std::error_code` overload should prevent the call from throwing exceptions. If for example the specified path is inaccessible due to permissions, the code should ignore the
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2103331130)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101317124
> in commit [5076d20](https://github.com/bitcoin/bitcoin/commit/5076d20fdb70a4bfafc4bdfe8293e347cb6bfa78): nit: since the error code `ec` isn't used, could use the shorter one-parameter variant of `is_regular_file`
The reason for doing this is the `std::error_code` overload should prevent the call from throwing exceptions. If for example the specified path is inaccessible due to permissions, the code should ignore the
...
💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103345132)
Made it this way to match the log line below, I applied your suggestion here and below.
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103345132)
Made it this way to match the log line below, I applied your suggestion here and below.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103345663)
This PR does that.
Without this PR, that test would not be possible as it is inherently contradictory. We want to test that we are able to get addresses from something for which we are not able to get addresses for in a watchonly wallet. The only way a watchonly wallet could have those addresses are if it had private keys and derived the keys already. But a watchonly wallet cannot have private keys. This situation is literally impossible to test prior to this PR.
The only reason such a tes
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103345663)
This PR does that.
Without this PR, that test would not be possible as it is inherently contradictory. We want to test that we are able to get addresses from something for which we are not able to get addresses for in a watchonly wallet. The only way a watchonly wallet could have those addresses are if it had private keys and derived the keys already. But a watchonly wallet cannot have private keys. This situation is literally impossible to test prior to this PR.
The only reason such a tes
...