💬 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
...
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3210927116)
Rebased 3843a15f5b5b50d47c2524f607ba0b01c90d42b1 -> 5537ac00983a9abd24689411b5d76058d7a02f1b ([spendblock_10](https://github.com/TheCharlatan/bitcoin/tree/spendblock_10) -> [spendblock_11](https://github.com/TheCharlatan/bitcoin/tree/spendblock_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_10..spendblock_11))
* Fixed conflict with #33078
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3210927116)
Rebased 3843a15f5b5b50d47c2524f607ba0b01c90d42b1 -> 5537ac00983a9abd24689411b5d76058d7a02f1b ([spendblock_10](https://github.com/TheCharlatan/bitcoin/tree/spendblock_10) -> [spendblock_11](https://github.com/TheCharlatan/bitcoin/tree/spendblock_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_10..spendblock_11))
* Fixed conflict with #33078
🚀 fanquake merged a pull request: "wallet: Remove isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523)
(https://github.com/bitcoin/bitcoin/pull/32523)
💬 mzumsande commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3210947431)
> 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 guaranteed to be flushed to disk. So it seems like even without reorgs, Init() can still trip up here with the issue reported in https://github.com/bitcoin/bitcoin/issues/33208?
Good point, I think you are right! This happens when the chaintip advances during the time when `Sync()`
...
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3210947431)
> 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 guaranteed to be flushed to disk. So it seems like even without reorgs, Init() can still trip up here with the issue reported in https://github.com/bitcoin/bitcoin/issues/33208?
Good point, I think you are right! This happens when the chaintip advances during the time when `Sync()`
...