💬 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-2637572312)
> If the binary does not share dependencies with the shared library, it should be safe.
The binary has to find `cs_main` somewhere though. And we don't want it adding its own.
I think I understand what's happening now and I'm testing an alternative.
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637572312)
> If the binary does not share dependencies with the shared library, it should be safe.
The binary has to find `cs_main` somewhere though. And we don't want it adding its own.
I think I understand what's happening now and I'm testing an alternative.
💬 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-2637577272)
> > 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`.
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637577272)
> > 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`.
💬 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?
(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.
(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 :)
(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.
(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`.
(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.
(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
...
(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
...
(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).
(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.
(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
(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)
(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.
(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
(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
(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.
(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
...
(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
(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