Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 ryanofsky commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-3210296557)
sipa, theuni, TheBlueMatt, and others raised a question in #31802 that wasn't directly considered here and is still relevant: **Should `bitcoind` accept an `-ipcbind` option?**

### Background

The multiprocess code and libmultiprocess/cap'nproto dependencies allow for two distinct features:

1. An *IPC listening* feature (`-ipcbind`), which lets the bitcoin node listen on a Unix socket and accept IPC connections from utilities and third-party applications.
2. A *process separation* feature, whi
...
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3210338335)
@Sjors or @sdaftuar: would you mind doing some light review of 1771e59f1a1fcaa6a1d79c93bc0c0c3b1003cf82? It represents the meat of this PR, making the headers sync state params that affect memory usage able to be configured for tests/other chains instead of hard-coded to mainnet conditions.

(davidgumberg also suggested (https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3208186878) reviewing 4aea5fa3690310bfc2617e3bf31f555dae7d9337 which also touches the `HeadersSyncState` implementat
...
💬 fanquake commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3210347297)
> Feature freeze (bug fixes only until release)

We are now in feature freeze.
💬 ryanofsky commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210390527)
IIUC, this change which toggles off the ENABLE_IPC option automatically when BUILD_FOR_FUZZING is set is safe for now because the fuzz test binary doesn't currently call any IPC code.

The change makes it more convenient to run the fuzz test without capnp installed since you don't have to turn off ENABLE_IPC manually.

But I think we would want to revert this change as soon as any fuzz test is written that does call IPC code, so it does seem like there may not much long term benefit in makin
...
💬 fanquake commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210404418)
> It would be good to have some fuzz tests exercising IPC code.

Is anyone working on that? I guess @Sjors?
💬 ryanofsky commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3210422658)
Code review ACK 2a815d126bc90f787f7adbe8cade45cb7307429b

Maybe we want to keep this PR open a little longer in case there are other small documentation updates like this? Seems fine to merge now though if preferred.

Another little thing I noticed is that italics should be removed from these files in files.md since they are now included in binary releases:

https://github.com/bitcoin/bitcoin/blob/7d9789401be4c91a9eb3c1112564a6524bdc4f70/doc/files.md?plain=1#L153-L154
💬 fjahr commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3210430071)
Concept ACK

Re [my concerns here](https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3194652080) that this might not address the full issue I am now more confident that this may be all there is to it. This is anecdotal evidence and not based on statistics but vague memories but it seems that it happens more often on testnets than on mainnet and they couldn't remember seeing it on signet ever. So this would make a lot of sense if the issues are always connected to reorgs.
💬 fanquake commented on issue "build: indefinite mpgen hang on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33176#issuecomment-3210436475)
Put on the v30.0 milestone.
💬 ryanofsky commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210452958)
> Is anyone working on that? I guess @Sjors?

I could put together something very basic like a fuzz test calling the `echoipc` RPC method. But there are a number of other loose ends since #31802 which should take priority so will probably not get to this very soon. @darosior has mentioned writing fuzz tests for IPC before which could be more meaningful, like fuzzing the IPC socket.
💬 janb84 commented on pull request "doc: Remove wrong and redundant doxygen tag":
(https://github.com/bitcoin/bitcoin/pull/33236#issuecomment-3210470881)
Is there a reason why this change is limited to only `feerate.h` ? The wrongful use of the tag is also in `coinselection.cpp` and `spend.h`. To much churn ?
💬 maflcko commented on pull request "doc: Remove wrong and redundant doxygen tag":
(https://github.com/bitcoin/bitcoin/pull/33236#issuecomment-3210545416)
Thx, done. Good catch actually calling `git grep 'param@'` to find all of them :sweat_smile:
💬 Sjors commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210571615)
Made a note to add a fuzz test if @darosior or @ryanofsky don't get around to it.

Would prefer to leave this ON unless such a PR doesn't land before branch-off. Maybe add a v30 milestone?
💬 Sjors commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3210575290)
> Maybe we want to keep this PR open a little longer in case there are other small documentation updates like this?

Yes, I was assuming there would be more things showing up over the next few days.
🤔 janb84 reviewed a pull request: "doc: Remove wrong and redundant doxygen tag"
(https://github.com/bitcoin/bitcoin/pull/33236#pullrequestreview-3140672937)
ACK 966666de9a6211b8748f43d682490c924e132e58

Housekeeping PR, this PR cleans up or fixes some wrongly used doxygen tags.
💬 maflcko commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210599985)
I think marcofleon may pick this up next month?
💬 Sjors commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-3210606461)
Another thing to consider, in the context of binary size, is if we want to drop `bitcoin-gui` for now, as I don't think it's very common for anyone to be connecting a miner to a GUI.
💬 dergoegge commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210721205)
Just leaving a few notes on fuzzing the IPC interfaces.

There are several approaches to fuzzing the IPC interface and not all of them make sense within our repo. If we add IPC fuzz tests in the repo they should not spawn separate processes and not do actual IPC over a socket. This is because the fuzz tests we have in our repo are designed to run in a single process.

We could add tests that call the c++ handler methods for the various interface functions either directly or through the capnp
...
💬 maflcko commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3210729248)
Also, on Fedora you need both packages, see https://github.com/maflcko/b-c-nightly/commit/14d34abdda4bbb327583f908742925b230944444.

Suggested patch:


```diff
diff --git a/doc/build-unix.md b/doc/build-unix.md
index 38b4496687..04f757ba52 100644
--- a/doc/build-unix.md
+++ b/doc/build-unix.md
@@ -122,7 +122,7 @@ User-Space, Statically Defined Tracing (USDT) dependencies:

Cap'n Proto is needed for IPC functionality (see [multiprocess.md](multiprocess.md)):

- sudo dnf instal
...
👍 dergoegge approved a pull request: "doc: update example bitcoin conf for 29.1rc2"
(https://github.com/bitcoin/bitcoin/pull/33234#pullrequestreview-3140823485)
ACK 65dc198d2cf7160f54a469a0c806232e83dc7599
👍 willcl-ark approved a pull request: "doc: update example bitcoin conf for 29.1rc2"
(https://github.com/bitcoin/bitcoin/pull/33234#pullrequestreview-3140836020)
ACK 65dc198d2cf7160f54a469a0c806232e83dc7599