💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-2500865562)
Can you add an example to `doc/descriptors.md`?
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-2500865562)
Can you add an example to `doc/descriptors.md`?
💬 jonatack commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2500872083)
Concept ACK. Per https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-11-26#1069602: "it turns out the owners of 1.2.3.4, 11.22.33.44 and 8.8.8.8, if they would bother, would know the IP address of every dev who runs the functional tests at home."
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2500872083)
Concept ACK. Per https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-11-26#1069602: "it turns out the owners of 1.2.3.4, 11.22.33.44 and 8.8.8.8, if they would bother, would know the IP address of every dev who runs the functional tests at home."
👍 instagibbs approved a pull request: "doc, test: more ephemeral dust follow-ups"
(https://github.com/bitcoin/bitcoin/pull/31371#pullrequestreview-2461615363)
ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
(https://github.com/bitcoin/bitcoin/pull/31371#pullrequestreview-2461615363)
ACK 160799d9135528dbdea40690f0bb0d56c6c4803a
💬 instagibbs commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858588114)
micro-nit, no need to invalidate acks
```suggestion
"""Creates a 1P1C package containing ephemeral dust by default. By default, the parent transaction
```
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858588114)
micro-nit, no need to invalidate acks
```suggestion
"""Creates a 1P1C package containing ephemeral dust by default. By default, the parent transaction
```
⚠️ theStack opened an issue: "RFC: support for writing UTXO set dump (`dumptxoutset` RPC) to a named pipe"
(https://github.com/bitcoin/bitcoin/issues/31373)
### Please describe the feature you'd like to see added.
While the primary obvious use-case for the `dumptxoutset` RPC is to create AssumeUTXO snapshots (to be distributed and loaded on newly created nodes via the `loadtxoutset` RPC later), it can also be useful as input for external tooling like converters to other UTXO set formats, e.g. https://github.com/bitcoin/bitcoin/pull/27432. For those, the intermediate step of writing a >10GB file to disk and then reading it again is wasteful and anno
...
(https://github.com/bitcoin/bitcoin/issues/31373)
### Please describe the feature you'd like to see added.
While the primary obvious use-case for the `dumptxoutset` RPC is to create AssumeUTXO snapshots (to be distributed and loaded on newly created nodes via the `loadtxoutset` RPC later), it can also be useful as input for external tooling like converters to other UTXO set formats, e.g. https://github.com/bitcoin/bitcoin/pull/27432. For those, the intermediate step of writing a >10GB file to disk and then reading it again is wasteful and anno
...
💬 Sjors commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2500931127)
Maybe add https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki to description.
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2500931127)
Maybe add https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki to description.
💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2500962838)
To fix a potential lifetime issue, it is probably helpful to replace `bpf_usdt_readarg_p()` (which isn't even documented) in:
https://github.com/bitcoin/bitcoin/blob/f7144b24be09e7db2173a0b15d73f99a08b98118/test/functional/interface_usdt_mempool.py#L103-L104
with `bpf_usdt_readarg()` followed by [`bpf_probe_read_user()`](https://github.com/iovisor/bcc/blob/master/docs/reference_guide.md#10-bpf_probe_read_user) or a [`bpf_probe_read_user_str()`](https://github.com/iovisor/bcc/blob/master/do
...
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2500962838)
To fix a potential lifetime issue, it is probably helpful to replace `bpf_usdt_readarg_p()` (which isn't even documented) in:
https://github.com/bitcoin/bitcoin/blob/f7144b24be09e7db2173a0b15d73f99a08b98118/test/functional/interface_usdt_mempool.py#L103-L104
with `bpf_usdt_readarg()` followed by [`bpf_probe_read_user()`](https://github.com/iovisor/bcc/blob/master/docs/reference_guide.md#10-bpf_probe_read_user) or a [`bpf_probe_read_user_str()`](https://github.com/iovisor/bcc/blob/master/do
...
💬 hebasto commented on issue "Unable to compile for test coverage on Nixos 24.05":
(https://github.com/bitcoin/bitcoin/issues/31087#issuecomment-2500966713)
@danielabrozzoni
Could you please post the full output of the `cmake -B build -DCMAKE_BUILD_TYPE=Coverage` command?
(https://github.com/bitcoin/bitcoin/issues/31087#issuecomment-2500966713)
@danielabrozzoni
Could you please post the full output of the `cmake -B build -DCMAKE_BUILD_TYPE=Coverage` command?
💬 0xB10C commented on issue "Tracepoint Interface Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/31274#issuecomment-2500981683)
There is also #27380 with discussion about a problem in the `interface_usdt_mempool.py` test.
https://github.com/bitcoin/bitcoin/blob/f7144b24be09e7db2173a0b15d73f99a08b98118/test/functional/interface_usdt_mempool.py#L317-L320
A possible fix might be https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2500962838 - but even if this doesn't fix #27380, it should probably be done.
(https://github.com/bitcoin/bitcoin/issues/31274#issuecomment-2500981683)
There is also #27380 with discussion about a problem in the `interface_usdt_mempool.py` test.
https://github.com/bitcoin/bitcoin/blob/f7144b24be09e7db2173a0b15d73f99a08b98118/test/functional/interface_usdt_mempool.py#L317-L320
A possible fix might be https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2500962838 - but even if this doesn't fix #27380, it should probably be done.
🤔 jonatack reviewed a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2461700619)
Light ACK 5766bbefa94d40f8d37639c2752961617d7a262a
Similar change to the larger one I reviewed in https://github.com/bitcoin/bitcoin/pull/26812.
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2461700619)
Light ACK 5766bbefa94d40f8d37639c2752961617d7a262a
Similar change to the larger one I reviewed in https://github.com/bitcoin/bitcoin/pull/26812.
💬 jonatack commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858640053)
I also wondered why this was moved. Not moving it would be preferable to save reviewers' time.
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858640053)
I also wondered why this was moved. Not moving it would be preferable to save reviewers' time.
💬 jonatack commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858648731)
2f6ce54212926f7f8642dfa5021f4ff089a4de61 Suggest not needlessly moving this to save reviewer time.
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858648731)
2f6ce54212926f7f8642dfa5021f4ff089a4de61 Suggest not needlessly moving this to save reviewer time.
🤔 hodlinator reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2461686975)
Code review a3a4d199e298a76725d0c0424195ae54c7e95fe0
Great to test more aspects of the protocol!
Why is this being removed/changed?
https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/src/test/script_standard_tests.cpp#L271-L276
As I understand it we were creating a taproot output (v1) using a compressed pubkey with the type-prefix prepended, meaning it was 33 bytes instead of the standard 32. Maybe you could add it back with a clearer comment?
---
Pu
...
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2461686975)
Code review a3a4d199e298a76725d0c0424195ae54c7e95fe0
Great to test more aspects of the protocol!
Why is this being removed/changed?
https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/src/test/script_standard_tests.cpp#L271-L276
As I understand it we were creating a taproot output (v1) using a compressed pubkey with the type-prefix prepended, meaning it was 33 bytes instead of the standard 32. Maybe you could add it back with a clearer comment?
---
Pu
...
💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182)
Would prefer:
- Capitalizing constant as suggested above.
- Clearly associating the scope of the new constant with the witness type by putting it inside `struct PayToAnchor`.
- Using `constexpr` & `array` to minimize dynamic memory use.
- (Might as well send member data into `Assume()`).
```C++
struct PayToAnchor : public WitnessUnknown
{
/** Witness program for output script */
static constexpr std::array<unsigned char, 2> PROGRAM{0x4e, 0x73};
static constexpr unsigned i
...
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182)
Would prefer:
- Capitalizing constant as suggested above.
- Clearly associating the scope of the new constant with the witness type by putting it inside `struct PayToAnchor`.
- Using `constexpr` & `array` to minimize dynamic memory use.
- (Might as well send member data into `Assume()`).
```C++
struct PayToAnchor : public WitnessUnknown
{
/** Witness program for output script */
static constexpr std::array<unsigned char, 2> PROGRAM{0x4e, 0x73};
static constexpr unsigned i
...
👍 dergoegge approved a pull request: "build: Fix coverage builds"
(https://github.com/bitcoin/bitcoin/pull/31337#pullrequestreview-2461824497)
tACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1
(https://github.com/bitcoin/bitcoin/pull/31337#pullrequestreview-2461824497)
tACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1
💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2501064149)
@achow101 @ryanofsky @glozow Would appreciate if one of you could take a look :)
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2501064149)
@achow101 @ryanofsky @glozow Would appreciate if one of you could take a look :)
💬 jonatack commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2501079577)
Is this fix related to the "no coverage data" results in https://corecheck.dev, or is that a different issue?
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2501079577)
Is this fix related to the "no coverage data" results in https://corecheck.dev, or is that a different issue?
💬 dergoegge commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2501117599)
> Is this fix related to the "no coverage data" results in https://corecheck.dev/ (i.e. https://corecheck.dev/bitcoin/bitcoin/pulls/31337), or is that a different issue?
I think the reason for "no coverage data" on corecheck's site for this PR is that this PR doesn't change any source files covered by any tests. It does show coverage changes under "Lost/Gained baseline coverage" but those are likely caused by non-determinism in the tests.
This PR was triggered by coverage builds on oss-fuz
...
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2501117599)
> Is this fix related to the "no coverage data" results in https://corecheck.dev/ (i.e. https://corecheck.dev/bitcoin/bitcoin/pulls/31337), or is that a different issue?
I think the reason for "no coverage data" on corecheck's site for this PR is that this PR doesn't change any source files covered by any tests. It does show coverage changes under "Lost/Gained baseline coverage" but those are likely caused by non-determinism in the tests.
This PR was triggered by coverage builds on oss-fuz
...
💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2501141395)
It might be useful if someone expands `doc/multisig-tutorial.md` to add a MuSig2 section. That doesn't have to go in this PR, but it will make testing and review easier. The functional test added in this PR can be used for inspiration.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2501141395)
It might be useful if someone expands `doc/multisig-tutorial.md` to add a MuSig2 section. That doesn't have to go in this PR, but it will make testing and review easier. The functional test added in this PR can be used for inspiration.
💬 hebasto commented on issue "CMake `CheckPIESupported` doesn't always work":
(https://github.com/bitcoin/bitcoin/issues/30771#issuecomment-2501164856)
The upstream issue has been [fixed](https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10034) for CMake 3.32.
(https://github.com/bitcoin/bitcoin/issues/30771#issuecomment-2501164856)
The upstream issue has been [fixed](https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10034) for CMake 3.32.