💬 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
...
💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103345691)
Borked a fixup commit, thanks.
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103345691)
Borked a fixup commit, thanks.
💬 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_r2103348928)
We don't have tests that check that the resulting parent descriptors exactly match, just that they are valid parent descriptors.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2103348928)
We don't have tests that check that the resulting parent descriptors exactly match, just that they are valid parent descriptors.
💬 davidgumberg commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103355444)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103355444)
Thanks, fixed.
👍 ismaelsadeeq approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2862510597)
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Further improvement can come after this as mentioned in the description, and also since I could not and no one reproduced the freeze I encountered in the unclean shutdown it is not a blocker to this.
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2862510597)
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Further improvement can come after this as mentioned in the description, and also since I could not and no one reproduced the freeze I encountered in the unclean shutdown it is not a blocker to this.
📝 achow101 opened a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593)
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked c
...
(https://github.com/bitcoin/bitcoin/pull/32593)
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked c
...
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103368896)
> To be clear, Cluster::IsOversized() returns whether the Cluster is oversized in general.
huh, I missed thanks for the clarity.
I think you can add this comment as well or anything that would make this explicit and clearer will be appreciated.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2103368896)
> To be clear, Cluster::IsOversized() returns whether the Cluster is oversized in general.
huh, I missed thanks for the clarity.
I think you can add this comment as well or anything that would make this explicit and clearer will be appreciated.
💬 fanquake commented on issue "seeds: seed.bitcoin.jonasschnelli.ch not returning results":
(https://github.com/bitcoin/bitcoin/issues/32590#issuecomment-2902578249)
Thanks mate.
(https://github.com/bitcoin/bitcoin/issues/32590#issuecomment-2902578249)
Thanks mate.
✅ fanquake closed an issue: "seeds: seed.bitcoin.jonasschnelli.ch not returning results"
(https://github.com/bitcoin/bitcoin/issues/32590)
(https://github.com/bitcoin/bitcoin/issues/32590)
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2103406828)
this looks much better, I will review again soon.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2103406828)
this looks much better, I will review again soon.