💬 enirox001 commented on issue "wallet: render BIP388 wallet policies":
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2945652066)
Thanks for the clarification! That's a crucial point about listdescriptors and the /** handling.
I agree completely that getdescriptorinfo is the right place to start. My plan is to focus on implementing the policy and keys fields there, by adding logic to parse the input descriptor into a BIP 388 policy if it's compatible.
I'll keep the idea of a dedicated listwalletpolicies RPC in mind for a follow-up contribution, as that seems like the cleanest way to expose stored wallet policies without
...
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2945652066)
Thanks for the clarification! That's a crucial point about listdescriptors and the /** handling.
I agree completely that getdescriptorinfo is the right place to start. My plan is to focus on implementing the policy and keys fields there, by adding logic to parse the input descriptor into a BIP 388 policy if it's compatible.
I'll keep the idea of a dedicated listwalletpolicies RPC in mind for a follow-up contribution, as that seems like the cleanest way to expose stored wallet policies without
...
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2129806417)
> which is referring to the loop in Chainstate::SetBlockFailureFlags
It's referring to `SetBlockFailureFlags` and `RecalculateBestHeader` which both loop over the block index.
> unnecessary database writes
I agree that this is a potential follow-up (to #31835, pinging @stratospher). It's probably not performance-critical, because it's also guarded by PoW (creating multiple PoW-valid headers that build on a failed block is very costly).
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2129806417)
> which is referring to the loop in Chainstate::SetBlockFailureFlags
It's referring to `SetBlockFailureFlags` and `RecalculateBestHeader` which both loop over the block index.
> unnecessary database writes
I agree that this is a potential follow-up (to #31835, pinging @stratospher). It's probably not performance-critical, because it's also guarded by PoW (creating multiple PoW-valid headers that build on a failed block is very costly).
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2129810763)
I rewrote the commit message - hoped it's clearer now.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2129810763)
I rewrote the commit message - hoped it's clearer now.
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2945690965)
[ed172d3](https://github.com/bitcoin/bitcoin/commit/ed172d3002da68a3ddbd5d13e7d3f0618c1378d4) to [1f09e37](https://github.com/bitcoin/bitcoin/commit/1f09e3754363382806cc1e3d6bed25cd235ac5f4):
Addressed feedback by @stickies-v and @ryanofsky - thanks!
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2945690965)
[ed172d3](https://github.com/bitcoin/bitcoin/commit/ed172d3002da68a3ddbd5d13e7d3f0618c1378d4) to [1f09e37](https://github.com/bitcoin/bitcoin/commit/1f09e3754363382806cc1e3d6bed25cd235ac5f4):
Addressed feedback by @stickies-v and @ryanofsky - thanks!
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129101313)
IIUC, I think we should explicitly pass a dummy signing provider here instead of `arg` because there can't be a hardened derivation at this level. Also, better to not pass the signing provider if we know for certain that it will not be used.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129101313)
IIUC, I think we should explicitly pass a dummy signing provider here instead of `arg` because there can't be a hardened derivation at this level. Also, better to not pass the signing provider if we know for certain that it will not be used.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129134556)
Interesting, I now understand why it's written in a double negation way.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129134556)
Interesting, I now understand why it's written in a double negation way.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129128674)
Slightly confused in this `else` block: for the case of ranged participants and non ranged musig derivation that has a `m_path` such as `/0/1/2`, it doesn't seem to do the derivation after getting the aggregate pubkey because I don't see `m_path` being used here like it is used in `m_aggregate_provider` above.
I don't see an explicit mention of this in BIP 390 as well, is this not a valid case to consider?
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129128674)
Slightly confused in this `else` block: for the case of ranged participants and non ranged musig derivation that has a `m_path` such as `/0/1/2`, it doesn't seem to do the derivation after getting the aggregate pubkey because I don't see `m_path` being used here like it is used in `m_aggregate_provider` above.
I don't see an explicit mention of this in BIP 390 as well, is this not a valid case to consider?
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129133487)
```diff
- Cannot have hardened hardened derivation
+ Cannot have hardened derivation
```
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129133487)
```diff
- Cannot have hardened hardened derivation
+ Cannot have hardened derivation
```
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129163516)
Aren't the participant pubkeys already derived in the loop on line 659? Here it seems to only aggregate the derived keys.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129163516)
Aren't the participant pubkeys already derived in the loop on line 659? Here it seems to only aggregate the derived keys.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129819178)
I'm sensing that this block of derivation of participants' keys should reside later in the `else` block starting on line 674. Reason being that in case of non ranged participants that leads to the presence of `m_aggregate_provider`, we don't really need to derive the participants' keys again and `pos` is effectively unused.
After moving this block down there, either we can derive the keys in the `if` block below with 0 `pos`, or preferably cache the participants' derived keys along with `m_ag
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129819178)
I'm sensing that this block of derivation of participants' keys should reside later in the `else` block starting on line 674. Reason being that in case of non ranged participants that leads to the presence of `m_aggregate_provider`, we don't really need to derive the participants' keys again and `pos` is effectively unused.
After moving this block down there, either we can derive the keys in the `if` block below with 0 `pos`, or preferably cache the participants' derived keys along with `m_ag
...
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129857969)
> Good point, these 2 structs in PSBT would need to be updated:
I believe there won't need to be any changes to PSBT. If a participant pubkey is duplicated, it is still valid to for each time the pubkey appears, to use the same pubnonce and the same partial sig for that nonce for each instance the pubkey appears. The only complication with this is if the duplicate pubkeys are different signers and produce different nonces and partial sigs as they may overwrite each other resulting a PSBT wit
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129857969)
> Good point, these 2 structs in PSBT would need to be updated:
I believe there won't need to be any changes to PSBT. If a participant pubkey is duplicated, it is still valid to for each time the pubkey appears, to use the same pubnonce and the same partial sig for that nonce for each instance the pubkey appears. The only complication with this is if the duplicate pubkeys are different signers and produce different nonces and partial sigs as they may overwrite each other resulting a PSBT wit
...
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129889627)
It's disallowed
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129889627)
It's disallowed
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129925030)
Even with aggregate derivation, `out` needs to be filled with participant pukey info. This loop is present in order to do that for both aggregate derivation and participant derivation.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129925030)
Even with aggregate derivation, `out` needs to be filled with participant pukey info. This loop is present in order to do that for both aggregate derivation and participant derivation.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129925291)
Added a test that has interleaved worst peers in a LimitOrphans call
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129925291)
Added a test that has interleaved worst peers in a LimitOrphans call
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129927509)
Implemented the general idea, i.e. keep trimming until this peer wouldn't be the next thing we pop from the heap. There was some iterator invalidation going on here
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129927509)
Implemented the general idea, i.e. keep trimming until this peer wouldn't be the next thing we pop from the heap. There was some iterator invalidation going on here
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129928431)
Good idea, done
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129928431)
Good idea, done
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129933928)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129933928)
Done
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129934262)
Done
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129934262)
Done
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129935247)
Changed the comment.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2129935247)
Changed the comment.
💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2945810827)
Can this be closed as fixed by [31933](https://github.com/bitcoin/bitcoin/pull/31933)?
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2945810827)
Can this be closed as fixed by [31933](https://github.com/bitcoin/bitcoin/pull/31933)?