💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637351868)
> Unless I'm missing something, I don't think @luke-jr's critique is specific to libmultiprocess, but rather his consistent position on including libraries.
I see this but I think I actually agree with (or at least sympathize with) Luke's position on libraries, and am trying to ensure more clean integration with this library than other libraries that we depend on like leveldb, univalue, libevent. This PR is not forking the ilbrary or make making local changes to it. It is not replacing its bu
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637351868)
> Unless I'm missing something, I don't think @luke-jr's critique is specific to libmultiprocess, but rather his consistent position on including libraries.
I see this but I think I actually agree with (or at least sympathize with) Luke's position on libraries, and am trying to ensure more clean integration with this library than other libraries that we depend on like leveldb, univalue, libevent. This PR is not forking the ilbrary or make making local changes to it. It is not replacing its bu
...
📝 Sjors opened a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802)
Have depends make libmultiprocess by default, which has the effect of including it in the release binaries. Except for Windows and OpenBSD which are not supported yet, the latter due to a fairly trivial upstream issue.
The initial main use case is to enable experimental support for the Mining IPC interface. A working example of a Stratum v2 Template Provider client using this interface can be found here: https://github.com/Sjors/bitcoin/pull/48.
Additionally the `bitcoin-node` and `bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/31802)
Have depends make libmultiprocess by default, which has the effect of including it in the release binaries. Except for Windows and OpenBSD which are not supported yet, the latter due to a fairly trivial upstream issue.
The initial main use case is to enable experimental support for the Mining IPC interface. A working example of a Stratum v2 Template Provider client using this interface can be found here: https://github.com/Sjors/bitcoin/pull/48.
Additionally the `bitcoin-node` and `bitcoi
...
💬 luke-jr commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637357864)
Downloading something separately is not a real issue.
But once it's a subtree, you can't avoid downloading it, and it might even be messy to remove it (to ensure it isn't used).
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637357864)
Downloading something separately is not a real issue.
But once it's a subtree, you can't avoid downloading it, and it might even be messy to remove it (to ensure it isn't used).
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2637377337)
All jobs happy!
I opened #31802 as draft which takes over the original intention for this PR. It flips the depends default and includes 743ae9b4e2746f48d707d49fc2c8ba908995034d which I dropped here.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2637377337)
All jobs happy!
I opened #31802 as draft which takes over the original intention for this PR. It flips the depends default and includes 743ae9b4e2746f48d707d49fc2c8ba908995034d which I dropped here.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637412067)
> But once it's a subtree, you can't avoid downloading it, and it might even be messy to remove it (to ensure it isn't used).
By design, it is not messy to remove. The subtree is referenced in exactly one [`add_subdirectory`](https://github.com/bitcoin/bitcoin/blob/99b7b857d3b8b9ce9bd7501e2480583369740c55/src/ipc/CMakeLists.txt#L8) call, and nowhere else in the cmake build. If the `-DWITH_LIBMULTIPROCESS=ON` is set the `add_subdirectory` call is not made and a `find_package` call is made inst
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637412067)
> But once it's a subtree, you can't avoid downloading it, and it might even be messy to remove it (to ensure it isn't used).
By design, it is not messy to remove. The subtree is referenced in exactly one [`add_subdirectory`](https://github.com/bitcoin/bitcoin/blob/99b7b857d3b8b9ce9bd7501e2480583369740c55/src/ipc/CMakeLists.txt#L8) call, and nowhere else in the cmake build. If the `-DWITH_LIBMULTIPROCESS=ON` is set the `add_subdirectory` call is not made and a `find_package` call is made inst
...
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2637415972)
Merge conflict is probably from #25832.
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2637415972)
Merge conflict is probably from #25832.
💬 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-2637460278)
> @theuni
>
> > Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.
>
> To clarify this discussion, which binary you are referring to—`bitcoind` or `bitcoin-chainstate`? I assume the latter, as the former uses the static kernel library.
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 proble
...
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2637460278)
> @theuni
>
> > Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.
>
> To clarify this discussion, which binary you are referring to—`bitcoind` or `bitcoin-chainstate`? I assume the latter, as the former uses the static kernel library.
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 proble
...
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2637463025)
Using BIP155 network IDs is a good idea! Planning to address https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1943088145 in a followup.
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2637463025)
Using BIP155 network IDs is a good idea! Planning to address https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1943088145 in a followup.
📝 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.