💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012003697)
A comment before the first loop would be nice stating `Validate multipath key path specifiers`
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012003697)
A comment before the first loop would be nice stating `Validate multipath key path specifiers`
🤔 glozow reviewed a pull request: "fuzz: Fix off-by-one in package_rbf target"
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2713587920)
ACK fa5674c264d91eb3a99fa74ace8a1b6be113c0a8
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2713587920)
ACK fa5674c264d91eb3a99fa74ace8a1b6be113c0a8
💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2751133325)
Re: https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2713105661
In the docs file, I would keep the `3` though. Explicitly stating the count of descriptors it will expand to is helpful.
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2751133325)
Re: https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2713105661
In the docs file, I would keep the `3` though. Explicitly stating the count of descriptors it will expand to is helpful.
✅ glozow closed an issue: "Add test coverage for getblocktemplate fees"
(https://github.com/bitcoin/bitcoin/issues/32053)
(https://github.com/bitcoin/bitcoin/issues/32053)
🚀 glozow merged a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897)
(https://github.com/bitcoin/bitcoin/pull/31897)
🤔 glozow reviewed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803#pullrequestreview-2713599065)
lgtm ACK a63bae6e281a9d4b0d4d6948550107683cf65d3b
(https://github.com/bitcoin/bitcoin/pull/31803#pullrequestreview-2713599065)
lgtm ACK a63bae6e281a9d4b0d4d6948550107683cf65d3b
💬 glozow commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r2012014088)
Worth noting this is pegged to the number at which `GatherClusters` halts (which would lead to `!IsReadyToCalculate`)
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r2012014088)
Worth noting this is pegged to the number at which `GatherClusters` halts (which would lead to `!IsReadyToCalculate`)
💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751151893)
Not sure what to make of the Win64 fuzz failure:
```
Assertion failed: DecodeBase64PSBT(psbt, random_string, error) == error.empty(), file
```
https://github.com/bitcoin/bitcoin/actions/runs/14055561051/job/39354164966?pr=32096#step:14:373
cc @maflcko
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751151893)
Not sure what to make of the Win64 fuzz failure:
```
Assertion failed: DecodeBase64PSBT(psbt, random_string, error) == error.empty(), file
```
https://github.com/bitcoin/bitcoin/actions/runs/14055561051/job/39354164966?pr=32096#step:14:373
cc @maflcko
💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751158862)
> Not sure what to make of the Win64 fuzz failure:
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r2011712656
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751158862)
> Not sure what to make of the Win64 fuzz failure:
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r2011712656
💬 maflcko commented on pull request "test: replace hardcoded fee with node relay fee based calculation":
(https://github.com/bitcoin/bitcoin/pull/32058#discussion_r2012027710)
```suggestion
```
Actually, `FEE` here isn't a fee rate at all, but an absolute fee.
So it seems fine to just remove the todo and and assert completely.
(https://github.com/bitcoin/bitcoin/pull/32058#discussion_r2012027710)
```suggestion
```
Actually, `FEE` here isn't a fee rate at all, but an absolute fee.
So it seems fine to just remove the todo and and assert completely.
💬 Sjors commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751192946)
cc @hebasto
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751192946)
cc @hebasto
🤔 ryanofsky reviewed a pull request: "multiprocess: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2713678101)
Updated 5554f9cb4abf7e1852dd79d2d75e714fa8cbeadd -> 0ec78961915da141b9c68a39ec0ebd5091c13be0 ([`pr/mine.20`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.20) -> [`pr/mine.21`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.21), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.20..pr/mine.21)) adding suggested test fix
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2713678101)
Updated 5554f9cb4abf7e1852dd79d2d75e714fa8cbeadd -> 0ec78961915da141b9c68a39ec0ebd5091c13be0 ([`pr/mine.20`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.20) -> [`pr/mine.21`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.21), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.20..pr/mine.21)) adding suggested test fix
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2012057155)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2007217890
Thanks! Applied fix
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2012057155)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2007217890
Thanks! Applied fix
💬 fjahr commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751257136)
re-ACK 2ee474d33f3340084a458e7348f5358fcaea8911
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2751257136)
re-ACK 2ee474d33f3340084a458e7348f5358fcaea8911
💬 laanwj commented on pull request "build: Remove bitness suffix from Windows installer":
(https://github.com/bitcoin/bitcoin/pull/32132#issuecomment-2751279047)
> We still have things like x86_64-w64-mingw32/bitcoin-94967c353ed8-win64-setup-unsigned.exe
i don't think this is what you mean here, `win64` could definitely just be `win`, but for completeness's sake: architecture tuples like `x86_64-w64-mingw32` are set in stone, there's no leeway to remove bitness where those are directly included.
(https://github.com/bitcoin/bitcoin/pull/32132#issuecomment-2751279047)
> We still have things like x86_64-w64-mingw32/bitcoin-94967c353ed8-win64-setup-unsigned.exe
i don't think this is what you mean here, `win64` could definitely just be `win`, but for completeness's sake: architecture tuples like `x86_64-w64-mingw32` are set in stone, there's no leeway to remove bitness where those are directly included.
👍 laanwj approved a pull request: "build: Remove bitness suffix from Windows installer"
(https://github.com/bitcoin/bitcoin/pull/32132#pullrequestreview-2713794902)
ACK fb2b05b1259d3e69e6e675adfa30b429424c7625
Thought we already made this change long ago, but apparently one was missed.
(https://github.com/bitcoin/bitcoin/pull/32132#pullrequestreview-2713794902)
ACK fb2b05b1259d3e69e6e675adfa30b429424c7625
Thought we already made this change long ago, but apparently one was missed.
🤔 hebasto reviewed a pull request: "cmake: Avoid fuzzer "multiple definition of `main'" errors"
(https://github.com/bitcoin/bitcoin/pull/31992#pullrequestreview-2713808140)
Approach ACK 60d2afe65484f755d99191bb650b9f9a784ee2c2, I have reviewed the code and it looks OK.
Tested on Ubuntu 24.10 and in the OSS-Fuzz environment.
As '_a separate "fuzzer_interface" with flags specifically intended to apply to the fuzz test binary_', it seems reasonable to place all `<COMMAND>(fuzzer_interface ...)` invocations under the existing `if(BUILD_FUZZ_BINARY)` condition.
nit: No need to quote variables here:
```diff
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -365,7
...
(https://github.com/bitcoin/bitcoin/pull/31992#pullrequestreview-2713808140)
Approach ACK 60d2afe65484f755d99191bb650b9f9a784ee2c2, I have reviewed the code and it looks OK.
Tested on Ubuntu 24.10 and in the OSS-Fuzz environment.
As '_a separate "fuzzer_interface" with flags specifically intended to apply to the fuzz test binary_', it seems reasonable to place all `<COMMAND>(fuzzer_interface ...)` invocations under the existing `if(BUILD_FUZZ_BINARY)` condition.
nit: No need to quote variables here:
```diff
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -365,7
...
💬 Chand-ra commented on pull request "test: replace hardcoded fee with node relay fee based calculation":
(https://github.com/bitcoin/bitcoin/pull/32058#discussion_r2012135129)
Makes sense.
(https://github.com/bitcoin/bitcoin/pull/32058#discussion_r2012135129)
Makes sense.
💬 hodlinator commented on pull request "build: Remove bitness suffix from Windows installer":
(https://github.com/bitcoin/bitcoin/pull/32132#issuecomment-2751319387)
> i don't think this is what you mean here, `win64` could definitely just be `win`, but for completeness's sake: architecture tuples like `x86_64-w64-mingw32` are set in stone, there's no leeway to remove bitness where those are directly included.
Was thinking `x86_64-windows-mingw` would be sufficient, and on par with `x86_64-linux-gnu`. What sets them in stone?
(https://github.com/bitcoin/bitcoin/pull/32132#issuecomment-2751319387)
> i don't think this is what you mean here, `win64` could definitely just be `win`, but for completeness's sake: architecture tuples like `x86_64-w64-mingw32` are set in stone, there's no leeway to remove bitness where those are directly included.
Was thinking `x86_64-windows-mingw` would be sufficient, and on par with `x86_64-linux-gnu`. What sets them in stone?
💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2751321498)
The new design also allows for using different keys and pubs in different tests instead of using one set in all the tests.
The overall commentary looks great, the commits can be cleaned up.
(https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2751321498)
The new design also allows for using different keys and pubs in different tests instead of using one set in all the tests.
The overall commentary looks great, the commits can be cleaned up.