💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2891831571)
@blockdyor Unclear to me why the other PR being closed in favor of this one implies this one should be closed. Going to keep this open, thanks!
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2891831571)
@blockdyor Unclear to me why the other PR being closed in favor of this one implies this one should be closed. Going to keep this open, thanks!
👍 ryanofsky approved 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-2851481522)
Code review ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce. This looks good and is a nice improvement. Left some suggestions below I think it could be good to follow up on, though
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2851481522)
Code review ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce. This looks good and is a nice improvement. Left some suggestions below I think it could be good to follow up on, though
💬 ryanofsky 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_r2096201788)
In commit "validation: use a lock for CCheckQueueControl" (da5d55227e6aead8a9fbb85e967801cfcd4283ce)
Something like LOCK_ARGS might be a better name for this macro than than IN_PLACE_LOCK, because it expands to a list of 4 arguments, not a lock. Also, TRY_LOCK and WAIT_LOCK above could be deduplicated and written using LOCK_ARGS.
I don't think there is a close parallel between IN_PLACE_LOCK and std::in_place other than that they both get used as constructor arguments. IN_PLACE_LOCK isn't b
...
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096201788)
In commit "validation: use a lock for CCheckQueueControl" (da5d55227e6aead8a9fbb85e967801cfcd4283ce)
Something like LOCK_ARGS might be a better name for this macro than than IN_PLACE_LOCK, because it expands to a list of 4 arguments, not a lock. Also, TRY_LOCK and WAIT_LOCK above could be deduplicated and written using LOCK_ARGS.
I don't think there is a close parallel between IN_PLACE_LOCK and std::in_place other than that they both get used as constructor arguments. IN_PLACE_LOCK isn't b
...
💬 ryanofsky 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_r2096166533)
In commit "validation: only create a CCheckQueueControl if it's actually going to be used" (bcde9153012b2ce3e9ff5b7cd1c6f9866453e727)
Seems like it would be good to change `parallel_script_checks ? &vChecks : nullptr` on line 2679 above to `control ? &vChecks : nullptr` to be checking for the `control` condition consistently in this function.
IMO, it would also be nice to have a comment here saying that if `control` is unset, checks were already performed in the `CheckInputScripts` call. O
...
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096166533)
In commit "validation: only create a CCheckQueueControl if it's actually going to be used" (bcde9153012b2ce3e9ff5b7cd1c6f9866453e727)
Seems like it would be good to change `parallel_script_checks ? &vChecks : nullptr` on line 2679 above to `control ? &vChecks : nullptr` to be checking for the `control` condition consistently in this function.
IMO, it would also be nice to have a comment here saying that if `control` is unset, checks were already performed in the `CheckInputScripts` call. O
...
💬 ryanofsky 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_r2096240944)
re: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2083592266
Following seems to prevent duplicate locking. Error message is:
```c++
src/bench/checkqueue.cpp:60:42: error: acquiring mutex 'queue.m_control_mutex' that is already held [-Werror,-Wthread-safety-analysis]
60 | CCheckQueueControl<PrevectorJob> control2(queue);
| ^
src/bench/checkqueue.cpp:59:42: note: mutex acquired here
59 | CCheckQueueControl<
...
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096240944)
re: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2083592266
Following seems to prevent duplicate locking. Error message is:
```c++
src/bench/checkqueue.cpp:60:42: error: acquiring mutex 'queue.m_control_mutex' that is already held [-Werror,-Wthread-safety-analysis]
60 | CCheckQueueControl<PrevectorJob> control2(queue);
| ^
src/bench/checkqueue.cpp:59:42: note: mutex acquired here
59 | CCheckQueueControl<
...
💬 ryanofsky 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_r2096161473)
In commit "validation: only create a CCheckQueueControl if it's actually going to be used" (bcde9153012b2ce3e9ff5b7cd1c6f9866453e727)
Developer notes want single-line if or multiple line if with braces, not multiline if without braces.
> - If an `if` only has a single-statement `then`-clause, it can appear
> on the same line as the `if`, without braces. In every other case,
> braces are required, and the `then` and `else` clauses must appear
> correctly indented on a new line.
...
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096161473)
In commit "validation: only create a CCheckQueueControl if it's actually going to be used" (bcde9153012b2ce3e9ff5b7cd1c6f9866453e727)
Developer notes want single-line if or multiple line if with braces, not multiline if without braces.
> - If an `if` only has a single-statement `then`-clause, it can appear
> on the same line as the `if`, without braces. In every other case,
> braces are required, and the `then` and `else` clauses must appear
> correctly indented on a new line.
...
💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096234268)
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: could `Assert` this condition instead, as otherwise the logic suggests that it's possible that neither of the if-branches are executed (it's not, IIUC; the only reason to not have a cached aggregate pubkey at this point is if we have ranged participants, so this condition is always true)
```suggestion
} else {
Assert(IsRangedParticipants());
```
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096234268)
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: could `Assert` this condition instead, as otherwise the logic suggests that it's possible that neither of the if-branches are executed (it's not, IIUC; the only reason to not have a cached aggregate pubkey at this point is if we have ranged participants, so this condition is always true)
```suggestion
} else {
Assert(IsRangedParticipants());
```
💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096247242)
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: not too fond of "reverse-hexstrings of byte-strings" creeping in, could do instead
```suggestion
using namespace util::hex_literals;
constexpr std::array<uint8_t, 32> MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
```
and assign the constant to the uint256 instance via `... = uint256(MUSIG_CHAINCODE)`.
(but shouldn't be a blocker, can be improved in a follow-up as well, if others are even as a
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096247242)
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: not too fond of "reverse-hexstrings of byte-strings" creeping in, could do instead
```suggestion
using namespace util::hex_literals;
constexpr std::array<uint8_t, 32> MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
```
and assign the constant to the uint256 instance via `... = uint256(MUSIG_CHAINCODE)`.
(but shouldn't be a blocker, can be improved in a follow-up as well, if others are even as a
...
💬 Sjors commented on issue "Awesome multisig PR labyrinth guide":
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-2891856702)
The MuSig2 party is now at #31244.
Meanwhile it's still very tedious to setup a multisig wallet, MuSig2 or otherwise. Suggested review to make progress is #29136 which introduces a `unused(KEY)` descriptor.
Building on that, I think we need a `gethdkey` (singular) RPC that can derive any xpub for an `unused(KEY)`, in particular `m/87'/0'/0'`.
After that we need to make the wallet a bit smarter when it imports a descriptor that contains an xpub for which it has the master key.
The final piece
...
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-2891856702)
The MuSig2 party is now at #31244.
Meanwhile it's still very tedious to setup a multisig wallet, MuSig2 or otherwise. Suggested review to make progress is #29136 which introduces a `unused(KEY)` descriptor.
Building on that, I think we need a `gethdkey` (singular) RPC that can derive any xpub for an `unused(KEY)`, in particular `m/87'/0'/0'`.
After that we need to make the wallet a bit smarter when it imports a descriptor that contains an xpub for which it has the master key.
The final piece
...
💬 ryanofsky commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891869792)
re: https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891771230
> > All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.
Sorry, I did not write that very clearly. I am not suggesting dropping the `interfaces::BlockTemplate` class and only exposing the `CBlockTemplate` struct. I am just suggesting adding a `const CBlockTemplate*` accessor to the interface
...
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891869792)
re: https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891771230
> > All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.
Sorry, I did not write that very clearly. I am not suggesting dropping the `interfaces::BlockTemplate` class and only exposing the `CBlockTemplate` struct. I am just suggesting adding a `const CBlockTemplate*` accessor to the interface
...
👍 ryanofsky approved a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2840890233)
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2840890233)
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2096279905)
In commit "wallet: Update best block record after block dis/connect" (7bacabb204b6c34f9545f0b37e2c66296ad2c0de)
IMO would be good to assert `!loc.IsNull()` as a sanity check to ensure that doesn't happen. (It shouldn't, but node or wallet code could change in the future and invalidate current assumptions.)
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2096279905)
In commit "wallet: Update best block record after block dis/connect" (7bacabb204b6c34f9545f0b37e2c66296ad2c0de)
IMO would be good to assert `!loc.IsNull()` as a sanity check to ensure that doesn't happen. (It shouldn't, but node or wallet code could change in the future and invalidate current assumptions.)
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2089361928)
Could also be good to check `!loc.IsNull()` in this function like that code is doing. (Could assert it or just not write to the database if it is null.)
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2089361928)
Could also be good to check `!loc.IsNull()` in this function like that code is doing. (Could assert it or just not write to the database if it is null.)
💬 Sjors commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891956944)
I agree it could just be an addition to the interface, though it also fits well if we want to prune it a bit.
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891956944)
I agree it could just be an addition to the interface, though it also fits well if we want to prune it a bit.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096297063)
Indeed it's nice if we can avoid this, because although reviewing the reversal was easy, finding it later with search is tedious.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096297063)
Indeed it's nice if we can avoid this, because although reviewing the reversal was easy, finding it later with search is tedious.
👍 darosior approved a pull request: "test: properly check for per-tx sigops limit"
(https://github.com/bitcoin/bitcoin/pull/32533#pullrequestreview-2851706605)
ACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
(https://github.com/bitcoin/bitcoin/pull/32533#pullrequestreview-2851706605)
ACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
💬 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_r2096309054)
Nice! I guess it manages to see through the references after all. My testing shows that:
```c++
auto& queue = m_chainman.GetCheckQueue();
CCheckQueueControl<CScriptCheck> control(queue);
CCheckQueueControl<CScriptCheck> control2(queue)
```
```c++
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
```
Both of those indeed work.
I suppose I came to the conclusion that the
...
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096309054)
Nice! I guess it manages to see through the references after all. My testing shows that:
```c++
auto& queue = m_chainman.GetCheckQueue();
CCheckQueueControl<CScriptCheck> control(queue);
CCheckQueueControl<CScriptCheck> control2(queue)
```
```c++
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
```
Both of those indeed work.
I suppose I came to the conclusion that the
...
💬 darosior commented on pull request "test: properly check for per-tx sigops limit":
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2891985815)
> against an accidental soft fork
You mean against a tightening of the rules, which in the context of standardness is harder than a loosening of the rules? :p
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2891985815)
> against an accidental soft fork
You mean against a tightening of the rules, which in the context of standardness is harder than a loosening of the rules? :p
👍 hebasto approved a pull request: "Use WitnessV0KeyHash in TestAddAddressesToSendBook"
(https://github.com/bitcoin-core/gui/pull/875#pullrequestreview-2851735518)
ACK fa982f14254433a969499e93c1c3f0db31dce6ab, I have reviewed the code along with the related changes from https://github.com/bitcoin/bitcoin/pull/32511.
(https://github.com/bitcoin-core/gui/pull/875#pullrequestreview-2851735518)
ACK fa982f14254433a969499e93c1c3f0db31dce6ab, I have reviewed the code along with the related changes from https://github.com/bitcoin/bitcoin/pull/32511.
✅ hebasto closed an issue: "`AddressBookTests` failure"
(https://github.com/bitcoin-core/gui/issues/874)
(https://github.com/bitcoin-core/gui/issues/874)