Bitcoin Core Github
42 subscribers
125K links
Download Telegram
💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012000014)
I like the suggested change, it's cleaner and easier to follow. There's a bit of validation in the second loop as well but I guess there is no way to completely separate the validation from parsing. The alternative would be to parse the multipath nums in the first loop as well but I would prefer this implementation over that.
💬 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`
🤔 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
💬 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.
glozow closed an issue: "Add test coverage for getblocktemplate fees"
(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)
🤔 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
💬 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`)
💬 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
💬 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
💬 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.
💬 Sjors commented on issue "fuzz: psbt_base64_decode fails on Windows":
(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
💬 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
💬 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
💬 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.
👍 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.
🤔 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
...
💬 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.
💬 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?