💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637580707)
> From the docs
>
> > The module definition file will be passed to the linker causing all symbols to be exported from the .dll. **For global data symbols, __declspec(dllimport) must still be used when compiling against the code in the .dll.** All other function symbols will be automatically exported and imported by callers
>
> See bold text. I suspect that's the real issue here, and that inlining is the opposite of what we want.

Mind sharing a link?

📝 jonatack opened a pull request: "doc: improve NODE_NETWORK_LIMITED documentation per BIP159"
(https://github.com/bitcoin/bitcoin/pull/31805)
`NODE_NETWORK_LIMITED` only signals a limited node if `NODE_NETWORK` is not set. See `IsLimitedPeer()` in `src/net_processing.cpp` for an example of this logic.

Also, per BIP159, the definition is:

NODE_NETWORK_LIMITED || bit 10 (0x400) || If signaled, the peer <I>MUST</I> be capable of serving at least the last 288 blocks (~2 days).

Clarify this better in our documentation.

💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637587851)
> > > If the binary does not share dependencies with the shared library, it should be safe.
> >
> >
> > The binary has to find `cs_main` somewhere though.
>
> Right. `bitcoin-chainstate` finds it in `libbitcoinkernel.so`.

Yes. But when inlining, it's free to generate its own as opposed to using the kernel's. Which is the opposite of what we want.

> Mind sharing a link?

Sorry, I meant to link there. Updated.

Give me a few min, I'm testing over here :)

💬 maflcko commented on pull request "ci: Remove no longer needed `-Wno-error=documentation`":
(https://github.com/bitcoin/bitcoin/pull/31804#issuecomment-2637613179)
lgtm ACK f1d7a6dfa1411ccf741fbf7351ea4f229dd1e63e

Not sure why and since when this is no longer needed. I am happy to bisect, if someone is interested.

📝 brunoerg opened a pull request: "fuzz: coinselection: cover `SetBumpFeeDiscount`"
(https://github.com/bitcoin/bitcoin/pull/31806)
`SetBumpFeeDiscount` sets the bump fee discount which is used to calculate the waste. We currently have no fuzz coverage for this function, so this PR adds it by calling `SetBumpFeeDiscount` before `RecalculateWaste`.

💬 hebasto commented on pull request "ci: Remove no longer needed `-Wno-error=documentation`":
(https://github.com/bitcoin/bitcoin/pull/31804#issuecomment-2637622756)
> Not sure why and since when this is no longer needed. I am happy to bisect, if someone is interested.

From https://github.com/bitcoin/bitcoin/commit/b088062e687d95deff28b0715fd4859449b56584 it follows that it comes "from Boost Test code". CMake adds dependencies' include directories with `-isystem`, which effectively silence warnings.

💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2637627778)
> > * verificationprogress = just blocks/headers
>
> I think the problem doing this is that not all blocks "are worth the same"

I think the suggestion is to keep the "tx weight" estimation, not change it. The suggestion is basically to split the "future headers estimation" from the verification progress estimation of a block in a known headers chain and return two floats.

(I wanted to suggest the same, because I was certain that this was suggested earlier already, but I couldn't find an
...

💬 hebasto commented on pull request "depends: Avoid using the `-ffile-prefix-map` compiler option":
(https://github.com/bitcoin/bitcoin/pull/31800#issuecomment-2637629490)
My Guix build:
```
aarch64
4ec7b4c22a31fa78161d7f826390e04a5ef50f04eba7c5e7eb5394166d613a16 guix-build-407062f2ac93/output/aarch64-linux-gnu/SHA256SUMS.part
7577f1d0847ab472e60b1d6e14e28ed442c36db190d1c214d41013b296581447 guix-build-407062f2ac93/output/aarch64-linux-gnu/bitcoin-407062f2ac93-aarch64-linux-gnu-debug.tar.gz
911518a3f02677a6a2ad0110c6b40b0e6f9789f30fa0f69229d39de54f565dfa guix-build-407062f2ac93/output/aarch64-linux-gnu/bitcoin-407062f2ac93-aarch64-linux-gnu.tar.gz
95858bf5
...

💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637644717)
Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

💬 maflcko commented on pull request "miniscript: convert non-critical asserts to CHECK_NONFATAL":
(https://github.com/bitcoin/bitcoin/pull/31727#discussion_r1943443912)
Just for reference, `CHECK_NONFATAL` is safe (from an UB perspective) to be used as a drop-in replacement for `assert/Assert` (and vice-versa) with only a slight difference: An assertion failure will abort the whole program, and not continue to the next statement. A nonfatal check failure will also not continue to the next statement, but it will just throw an exception and not fatally abort the whole program.

👍 l0rinc approved a pull request: "feefrac: add support for evaluating at given size"
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2596619215)
Rebase didn't result in any code change, but CI compiles fuzzing for every commit now

💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1943451470)
Now that Fuzz is always compiled, this should likely be move to `feefrac: add helper functions for 96-bit division` (see https://github.com/bitcoin/bitcoin/actions/runs/13162889382/job/36735875466?pr=30535#step:6:2772)

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1943467899)
Done.

💬 l0rinc commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2637727127)
Another one https://github.com/bitcoin/bitcoin/pull/30535/checks?check_run_id=36740042246

💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637729330)
ACK 5600091e8384a69cfc7e95a7aea816e7a9076917

Build failure will likely be resolved by retriggering it later

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637740773)
Added a few commit messages to explain the rationale for the changes more.

💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2637754217)
> Interesting. It would be nice to reduce this

So I reduced the failure down to a standalone case, but it could be reduced further. Now it is unclear to me if there is a compiler bug here or not, because this is caused by a very specific interaction between the two compiler versions we are using:

- gcc version Ubuntu 13.3.0-6ubuntu2~24.04
- clang version 18.1.3 (1ubuntu1)

and maybe the specific flags we are using. The bug happens because the linker links together the

- `_ZN2kj4ctorIjIjEEEvRT
...

💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637792608)
ACK f6aa28cf8fad6a3240498b500524bb380855b18e

Added build failure to https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2637727127 - should be retriggered later

💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2637794665)
Following seem to be minimal flags to reproduce this issue. None of the other flags except for `-fPIC` was making any difference, and without `-fPIC` the program just segfaults instead of printing the wrong value:

```bash
g++ -m32 -fPIC -c test_gcc.cpp -o test_gcc.o
clang++ -m32 -fPIC -c test_clang.cpp -o test_clang.o
clang++ -m32 -fPIC test_gcc.o test_clang.o
./a.out
```

So it seems like this version of gcc and clang just always (regardless of flags) assume incompatible calling conventions fo
...

💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2637796858)
I don't understand the changes here. Instead of repeating what the changes are in the pull description, it would be good to explain why they are done and why the downsides are acceptable.

I guess this deprecates an RPC without replacement, without release notes, without an explanation or long-term plan?

If yes, then I tend toward NACK.