💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2093026925)
might be worth getting this one mined to show it's consensus-valid still
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2093026925)
might be worth getting this one mined to show it's consensus-valid still
📝 darosior opened a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530)
32-bit architecture is limited to 4GiB of RAM, so it doesn't make sense to set a too high value.
(https://github.com/bitcoin/bitcoin/pull/32530)
32-bit architecture is limited to 4GiB of RAM, so it doesn't make sense to set a too high value.
🤔 brunoerg reviewed a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2846644623)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2846644623)
Concept ACK
🤔 rkrux reviewed a pull request: "test: Remove legacy wallet RPC overloads"
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2846350523)
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
`RPCOverloadWrapper` did catch my eye and made me wonder why it was added in the first place when I reviewed #32360 as it added an additional layer for calling the RPCs in the test framework - good that it can be removed after removal of legacy wallets, makes it slightly easier to track the flow in the framework.
> On the second commit, [test: Replace importprivkey with wallet_importprivkey](https://github.com/bitcoin/bitcoin/pull/32452/commits/
...
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2846350523)
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
`RPCOverloadWrapper` did catch my eye and made me wonder why it was added in the first place when I reviewed #32360 as it added an additional layer for calling the RPCs in the test framework - good that it can be removed after removal of legacy wallets, makes it slightly easier to track the flow in the framework.
> On the second commit, [test: Replace importprivkey with wallet_importprivkey](https://github.com/bitcoin/bitcoin/pull/32452/commits/
...
💬 rkrux commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2093072864)
Some function documentation here could prove to be helpful for others who are navigating the tests. Might even encourage to create more such helper/wrapper functions as needed.
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2093072864)
Some function documentation here could prove to be helpful for others who are navigating the tests. Might even encourage to create more such helper/wrapper functions as needed.
💬 rkrux commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2092869629)
Mentioning for clarity:
Fine to remove but as this test stood before this PR, `w1` doesn't seem to be a legacy wallet as I can see `descriptors` argument was not set to false in its creation. Maybe it was in some older diff.
(https://github.com/bitcoin/bitcoin/pull/32452#discussion_r2092869629)
Mentioning for clarity:
Fine to remove but as this test stood before this PR, `w1` doesn't seem to be a legacy wallet as I can see `descriptors` argument was not set to false in its creation. Maybe it was in some older diff.
💬 rkrux commented on pull request "refactor: bdb removals":
(https://github.com/bitcoin/bitcoin/pull/32511#issuecomment-2886778893)
Oh yeah, for a second there, I got engrossed in that wrapper/helper function over there and forgot it's in the functional test framework.
(https://github.com/bitcoin/bitcoin/pull/32511#issuecomment-2886778893)
Oh yeah, for a second there, I got engrossed in that wrapper/helper function over there and forgot it's in the functional test framework.
💬 ismaelsadeeq commented on pull request "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/32509#issuecomment-2886789925)
Code review and tested ACK bf950c45
This PR makes the test easier to read and also rightly fixes the issue in the OP.
(https://github.com/bitcoin/bitcoin/pull/32509#issuecomment-2886789925)
Code review and tested ACK bf950c45
This PR makes the test easier to read and also rightly fixes the issue in the OP.
💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093101180)
Thanks for the context.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093101180)
Thanks for the context.
⚠️ instagibbs opened an issue: "Change functional tests to trigger reorg by reconsiderblock"
(https://github.com/bitcoin/bitcoin/issues/32531)
### Motivation
While working on https://github.com/bitcoin/bitcoin/pull/32516 I found that for testing mempool entry from reorged blocks, directly using invalidateblock to trigger the reorg has at least a couple behaviors that don't match real reorgs:
1) Only allows 10 deep reorg before it stops trying to re-enter things into the mempool
2) doesn't respect descendant chain limits(? still unsure how this is happening)
the code paths greatly diverge from normal re-orgs, which makes this problem
...
(https://github.com/bitcoin/bitcoin/issues/32531)
### Motivation
While working on https://github.com/bitcoin/bitcoin/pull/32516 I found that for testing mempool entry from reorged blocks, directly using invalidateblock to trigger the reorg has at least a couple behaviors that don't match real reorgs:
1) Only allows 10 deep reorg before it stops trying to re-enter things into the mempool
2) doesn't respect descendant chain limits(? still unsure how this is happening)
the code paths greatly diverge from normal re-orgs, which makes this problem
...
🤔 Sjors reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2846300829)
Concept ACK
Reviewed up to 6b634754d2783ffe16570ad37dbcf9251a7efc24, mostly happy.
I didn't thoroughly check that all test vectors from BIP390 are in the test, but did check a few.
> Can you add an example to `doc/descriptors.md`?
This still seems useful (and this document should probably link back to the bip numbers, but that's orthogonal).
Manual testing hint for other reviewers: you can use the `deriveaddresses` RPC with some of the test vectors. Just use `00000000` as the chec
...
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2846300829)
Concept ACK
Reviewed up to 6b634754d2783ffe16570ad37dbcf9251a7efc24, mostly happy.
I didn't thoroughly check that all test vectors from BIP390 are in the test, but did check a few.
> Can you add an example to `doc/descriptors.md`?
This still seems useful (and this document should probably link back to the bip numbers, but that's orthogonal).
Manual testing hint for other reviewers: you can use the `deriveaddresses` RPC with some of the test vectors. Just use `00000000` as the chec
...
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092839369)
In 225b3adbf53e4dfde32a1f798cde30cc41998e3c "XOnlyPubKey: Add GetCPubKeys": maybe document:
```cpp
// Return both a version prefixed with 0x02, and one with 0x03.
```
The comment inside `GetKeyIDs` could also be moved to the header.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092839369)
In 225b3adbf53e4dfde32a1f798cde30cc41998e3c "XOnlyPubKey: Add GetCPubKeys": maybe document:
```cpp
// Return both a version prefixed with 0x02, and one with 0x03.
```
The comment inside `GetKeyIDs` could also be moved to the header.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092880889)
In 4a1eeee27a64c4be7293740f8fff839879b88d86 "util/string: Allow Split to include the separator": the commit message would be more clear if you use the same wording as in this comment, i.e. "include the separator at the end of the left side of the splits".
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092880889)
In 4a1eeee27a64c4be7293740f8fff839879b88d86 "util/string: Allow Split to include the separator": the commit message would be more clear if you use the same wording as in this comment, i.e. "include the separator at the end of the left side of the splits".
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092871907)
In fe02d7cb237c00de6abe1776b3101342ffddf757 "script/parsing: Allow Const to not skip the found constant": what does "sp" stand for anyway?
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092871907)
In fe02d7cb237c00de6abe1776b3101342ffddf757 "script/parsing: Allow Const to not skip the found constant": what does "sp" stand for anyway?
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092892121)
In 26c25fed919d2520f561a891236220d54880b079 "descriptors: Add PubkeyProvider::IsBIP32()": "can be" a BIP 32 extended key or "is"? In the former case, an additional comment would be useful.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092892121)
In 26c25fed919d2520f561a891236220d54880b079 "descriptors: Add PubkeyProvider::IsBIP32()": "can be" a BIP 32 extended key or "is"? In the former case, an additional comment would be useful.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092887857)
Also in https://github.com/bitcoin/bitcoin/commit/4a1eeee27a64c4be7293740f8fff839879b88d86, this would be a bit easier to read:
```
* will return the following strings:
* - foo(bar(1),
* - 2),
* - 3)
```
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092887857)
Also in https://github.com/bitcoin/bitcoin/commit/4a1eeee27a64c4be7293740f8fff839879b88d86, this would be a bit easier to read:
```
* will return the following strings:
* - foo(bar(1),
* - 2),
* - 3)
```
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092998484)
In 895f0c5ca9a12df843f2f7faff37c948046654ea "Add MuSig2 Keyagg Cache helper functions": although we don't have to copy-paste all libsecp documentation, I think it's worth mentioning that this doesn't sort pubkeys and the order matters.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092998484)
In 895f0c5ca9a12df843f2f7faff37c948046654ea "Add MuSig2 Keyagg Cache helper functions": although we don't have to copy-paste all libsecp documentation, I think it's worth mentioning that this doesn't sort pubkeys and the order matters.
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092904357)
In 895f0c5ca9a12df843f2f7faff37c948046654ea "Add MuSig2 Keyagg Cache helper functions": why is this plural? IIUC there's one aggregate _key_ that's derived from the participant _keys_. If so, then maybe call this `MuSig2DeriveAggregatePubkey()`
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2092904357)
In 895f0c5ca9a12df843f2f7faff37c948046654ea "Add MuSig2 Keyagg Cache helper functions": why is this plural? IIUC there's one aggregate _key_ that's derived from the participant _keys_. If so, then maybe call this `MuSig2DeriveAggregatePubkey()`
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093043309)
In 6b634754d2783ffe16570ad37dbcf9251a7efc24 "descriptor: Add MuSigPubkeyProvider": could add a short comment to explain why this doesn't exist
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093043309)
In 6b634754d2783ffe16570ad37dbcf9251a7efc24 "descriptor: Add MuSigPubkeyProvider": could add a short comment to explain why this doesn't exist
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093043962)
In 6b634754d2783ffe16570ad37dbcf9251a7efc24 "descriptor: Add MuSigPubkeyProvider": do you mean "all or nothing" or "any that we have"?
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2093043962)
In 6b634754d2783ffe16570ad37dbcf9251a7efc24 "descriptor: Add MuSigPubkeyProvider": do you mean "all or nothing" or "any that we have"?