💬 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"?
📝 l0rinc opened a pull request: "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532)
### Draft — requesting feedback on the overall direction
---
### Summary
For the vast majority of scripts we encounter, the sig-op count can be deduced directly from their fixed-length template (e.g. P2PKH, P2SH, P2WPKH, P2WSH, P2TR …), without scanning every opcode.
### Context
While profiling the remaining **SwiftSync** bottlenecks, I saw that [Own Samples](https://youtrack.jetbrains.com/issue/CPP-30003) highlighted `GetSigOpCount` as a non-negligible slice of validation time.
...
(https://github.com/bitcoin/bitcoin/pull/32532)
### Draft — requesting feedback on the overall direction
---
### Summary
For the vast majority of scripts we encounter, the sig-op count can be deduced directly from their fixed-length template (e.g. P2PKH, P2SH, P2WPKH, P2WSH, P2TR …), without scanning every opcode.
### Context
While profiling the remaining **SwiftSync** bottlenecks, I saw that [Own Samples](https://youtrack.jetbrains.com/issue/CPP-30003) highlighted `GetSigOpCount` as a non-negligible slice of validation time.
...
💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
E.g on this branch
```
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/bitcoin ((a5ac43d9))> build/bin/bitcoin -h rpc
Usage: build/bin/bitcoin [OPTIONS] COMMAND...
Options:
-m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
-v, --version Show version information
-h, --help Show this help message
Commands:
gui [ARGS] Start GUI, equivale
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
E.g on this branch
```
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/bitcoin ((a5ac43d9))> build/bin/bitcoin -h rpc
Usage: build/bin/bitcoin [OPTIONS] COMMAND...
Options:
-m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
-v, --version Show version information
-h, --help Show this help message
Commands:
gui [ARGS] Start GUI, equivale
...
💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093117141)
fair point.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093117141)
fair point.
💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2886824545)
I have pushed the mentioned `SigOps` calculation optimization, adding the `/*fAccurate=*/` argument to each usage, and renamed `GetSigOpCount` to `GetLegacySigOpCount` to highlight how this differs from the post-segwit implementation.
I've also covered it with an extensive suite of tests and a benchmark, see: https://github.com/bitcoin/bitcoin/pull/32532
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2886824545)
I have pushed the mentioned `SigOps` calculation optimization, adding the `/*fAccurate=*/` argument to each usage, and renamed `GetSigOpCount` to `GetLegacySigOpCount` to highlight how this differs from the post-segwit implementation.
I've also covered it with an extensive suite of tests and a benchmark, see: https://github.com/bitcoin/bitcoin/pull/32532
💬 willcl-ark commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2886830462)
OK I updated that branch a little, it is now simplified, and works for Qt. However I did have to carve out Boost (bypassing the provider), for reasons that are not totally clear to me still.
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2886830462)
OK I updated that branch a little, it is now simplified, and works for Qt. However I did have to carve out Boost (bypassing the provider), for reasons that are not totally clear to me still.
💬 hebasto commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2886839571)
> > From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE
>
> `_GNU_SOURCE` isn't supposed to ever be defined by default, and I don't see us defining it anywhere...?
Do we use GNU extensions?
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2886839571)
> > From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE
>
> `_GNU_SOURCE` isn't supposed to ever be defined by default, and I don't see us defining it anywhere...?
Do we use GNU extensions?