💬 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
(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.
(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.
(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.
(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 ?
(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:
(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?
(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.
(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.
(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?
(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.
(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
...
(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
...
(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
(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
(https://github.com/bitcoin/bitcoin/pull/33234#pullrequestreview-3140836020)
ACK 65dc198d2cf7160f54a469a0c806232e83dc7599
🚀 fanquake merged a pull request: "doc: update example bitcoin conf for 29.1rc2"
(https://github.com/bitcoin/bitcoin/pull/33234)
(https://github.com/bitcoin/bitcoin/pull/33234)
👍 stickies-v approved a pull request: "index: Don't commit state in BaseIndex::Rewind"
(https://github.com/bitcoin/bitcoin/pull/33212#pullrequestreview-3140857089)
ACK 4625b4b8f772de1cd852003645c75bfa68c6735a
Delaying the `Commit()` to a `ChainStateFlushed()` event seems like a safe way to (mostly?) fix the reported issue, with the only downside I can see potentially having to do a tiny bit of duplicated work during `Init()`.
It seems to me like it might not fully fix the issue. In `BaseIndex::Sync()`, we'll `Commit()` ever `SYNC_LOCATOR_WRITE_INTERVAL` (30s). `NextSyncBlock()` will get us all the way up to the chaintip, which I think is not guarante
...
(https://github.com/bitcoin/bitcoin/pull/33212#pullrequestreview-3140857089)
ACK 4625b4b8f772de1cd852003645c75bfa68c6735a
Delaying the `Commit()` to a `ChainStateFlushed()` event seems like a safe way to (mostly?) fix the reported issue, with the only downside I can see potentially having to do a tiny bit of duplicated work during `Init()`.
It seems to me like it might not fully fix the issue. In `BaseIndex::Sync()`, we'll `Commit()` ever `SYNC_LOCATOR_WRITE_INTERVAL` (30s). `NextSyncBlock()` will get us all the way up to the chaintip, which I think is not guarante
...
💬 ryanofsky commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210805936)
Thanks @dergoegge this is really helpful. It's been hard for me to think about what would constitute a meaningful fuzz test, and it still may not be exactly clear, but this provides a great framework. I just have two question
> they should not spawn separate processes and not do actual IPC over a socket
Not spawning separate processes makes sense, but I assumed it might be ok to communicate across an internal sockiet, e.g. with [socketpair](https://man7.org/linux/man-pages/man2/socketpair.
...
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210805936)
Thanks @dergoegge this is really helpful. It's been hard for me to think about what would constitute a meaningful fuzz test, and it still may not be exactly clear, but this provides a great framework. I just have two question
> they should not spawn separate processes and not do actual IPC over a socket
Not spawning separate processes makes sense, but I assumed it might be ok to communicate across an internal sockiet, e.g. with [socketpair](https://man7.org/linux/man-pages/man2/socketpair.
...
👍 stickies-v approved a pull request: "miner: clamp options instead of asserting"
(https://github.com/bitcoin/bitcoin/pull/33222#pullrequestreview-3141002296)
ACK 7392b8b084be14b75536887b7ff216152d0a3307
> but I think in this case, just correcting to reasonable values is acceptable.
I agree. I generally support logging in situations like this, so that we at least give the user a chance to see something unexpected is happening without being unergonomic, but I think it's also fine as-is. We generally don't log for clamping, as far as I can tell.
(https://github.com/bitcoin/bitcoin/pull/33222#pullrequestreview-3141002296)
ACK 7392b8b084be14b75536887b7ff216152d0a3307
> but I think in this case, just correcting to reasonable values is acceptable.
I agree. I generally support logging in situations like this, so that we at least give the user a chance to see something unexpected is happening without being unergonomic, but I think it's also fine as-is. We generally don't log for clamping, as far as I can tell.
💬 dergoegge commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210906292)
> Not spawning separate processes makes sense, but I assumed it might be ok to communicate across an internal sockiet, e.g. with [socketpair](https://man7.org/linux/man-pages/man2/socketpair.2.html)? That is what a lot of IPC unit tests are doing.
I'd advise against this (e.g. does this work for all OSs in CI?, might be slower) but it might also be fine.
> So I'd be curious if it would affect the recommended fuzzing strategy if we did offer untrusted interfaces or decide to make the mining
...
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210906292)
> Not spawning separate processes makes sense, but I assumed it might be ok to communicate across an internal sockiet, e.g. with [socketpair](https://man7.org/linux/man-pages/man2/socketpair.2.html)? That is what a lot of IPC unit tests are doing.
I'd advise against this (e.g. does this work for all OSs in CI?, might be slower) but it might also be fine.
> So I'd be curious if it would affect the recommended fuzzing strategy if we did offer untrusted interfaces or decide to make the mining
...