📝 fjahr opened a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803)
This is follow-up to #31384 which should be merged shortly. Only the last commit is new.
It expands the fuzz test to cover the full range of allow block sizes in `BlockAssembler` by utilizing the `block_adjusted_max_weight` option if necessary.
See also the brief discussion here for context: https://github.com/bitcoin/bitcoin/pull/31384/files#r1942039833
(https://github.com/bitcoin/bitcoin/pull/31803)
This is follow-up to #31384 which should be merged shortly. Only the last commit is new.
It expands the fuzz test to cover the full range of allow block sizes in `BlockAssembler` by utilizing the `block_adjusted_max_weight` option if necessary.
See also the brief discussion here for context: https://github.com/bitcoin/bitcoin/pull/31384/files#r1942039833
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1943316627)
Done in #31803
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1943316627)
Done in #31803
👋 brunoerg's pull request is ready for review: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)
(https://github.com/bitcoin/bitcoin/pull/29694)
💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2637502854)
Just fixed CI, ready for review!
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2637502854)
Just fixed CI, ready for review!
💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2637520847)
> * verificationprogress = just blocks/headers
I think the problem doing this is that not all blocks "are worth the same". So when doing IBD we would go fast to high % and then slow down a lot because newer blocks have more transactions. If we want the % to be accurate by the time we should still measure it by txs.
E.g. The first 50k blocks would be around 5% using this approach, but it takes just 1-2min to sync them as they are empty. This could lead to a fake feeling of fast sync.
Also, w
...
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2637520847)
> * verificationprogress = just blocks/headers
I think the problem doing this is that not all blocks "are worth the same". So when doing IBD we would go fast to high % and then slow down a lot because newer blocks have more transactions. If we want the % to be accurate by the time we should still measure it by txs.
E.g. The first 50k blocks would be around 5% using this approach, but it takes just 1-2min to sync them as they are empty. This could lead to a fake feeling of fast sync.
Also, w
...
💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943343041)
NoteToSelf: Check this before next push
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943343041)
NoteToSelf: Check this before next push
💬 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-2637550455)
> What I'm building isn't really relevant... the combo of the inline var with hidden visibility when built as a shared lib is a potential problem.
I think it could be problematic when an inline variable with hidden visibility is part of a static library, which is then linked both to a binary and to a shared library. If the binary does not share dependencies with the shared library, it should be safe.
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637550455)
> What I'm building isn't really relevant... the combo of the inline var with hidden visibility when built as a shared lib is a potential problem.
I think it could be problematic when an inline variable with hidden visibility is part of a static library, which is then linked both to a binary and to a shared library. If the binary does not share dependencies with the shared library, it should be safe.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637554110)
Rebased because of datedness.
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2637554110)
Rebased because of datedness.
✅ hebasto closed a pull request: "ci: Replace `CMAKE_CXX_FLAGS` with `APPEND_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/31726)
(https://github.com/bitcoin/bitcoin/pull/31726)
📝 hebasto opened a pull request: "ci: Remove no longer needed '-Wno-error=documentation'"
(https://github.com/bitcoin/bitcoin/pull/31804)
Picked from https://github.com/bitcoin/bitcoin/pull/31726.
(https://github.com/bitcoin/bitcoin/pull/31804)
Picked from https://github.com/bitcoin/bitcoin/pull/31726.
💬 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-2637569405)
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.
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637569405)
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.
💬 hebasto commented on pull request "ci: Replace `CMAKE_CXX_FLAGS` with `APPEND_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/31726#discussion_r1943372430)
Sure! Please see https://github.com/bitcoin/bitcoin/pull/31804.
(https://github.com/bitcoin/bitcoin/pull/31726#discussion_r1943372430)
Sure! Please see https://github.com/bitcoin/bitcoin/pull/31804.
💬 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
...