💬 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()`
...
👍 ryanofsky approved a pull request: "miner: clamp options instead of asserting"
(https://github.com/bitcoin/bitcoin/pull/33222#pullrequestreview-3141107720)
Code review ACK 7392b8b084be14b75536887b7ff216152d0a3307. I think ideally this would throw an exception and return a clear error to the caller, or maybe log as stickies suggested, but clamping is much better than crashing.
(https://github.com/bitcoin/bitcoin/pull/33222#pullrequestreview-3141107720)
Code review ACK 7392b8b084be14b75536887b7ff216152d0a3307. I think ideally this would throw an exception and return a clear error to the caller, or maybe log as stickies suggested, but clamping is much better than crashing.
👍 pablomartin4btc approved a pull request: "doc: Remove wrong and redundant doxygen tag"
(https://github.com/bitcoin/bitcoin/pull/33236#pullrequestreview-3141191001)
ACK 966666de9a6211b8748f43d682490c924e132e58
Checked that there aren't more pending corrections left for `@param` (and for others by looking for "`@[`").
The corrections match the specification in the doxygen-compatible comments [section](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-doxygen-compatible-comments) on the dev-notes and in doxygen itself on the [param command](https://www.doxygen.nl/manual/commands.html#cmdparam).
(https://github.com/bitcoin/bitcoin/pull/33236#pullrequestreview-3141191001)
ACK 966666de9a6211b8748f43d682490c924e132e58
Checked that there aren't more pending corrections left for `@param` (and for others by looking for "`@[`").
The corrections match the specification in the doxygen-compatible comments [section](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-doxygen-compatible-comments) on the dev-notes and in doxygen itself on the [param command](https://www.doxygen.nl/manual/commands.html#cmdparam).
💬 maflcko commented on issue "depends: `native_libmultiprocess` fails to build on OpenBSD":
(https://github.com/bitcoin/bitcoin/issues/33219#issuecomment-3211086241)
upstream fix (from the previous comment) is at https://github.com/bitcoin-core/libmultiprocess/pull/195
(https://github.com/bitcoin/bitcoin/issues/33219#issuecomment-3211086241)
upstream fix (from the previous comment) is at https://github.com/bitcoin-core/libmultiprocess/pull/195
💬 ryanofsky commented on pull request "miner: clamp options instead of asserting":
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3211088504)
@Sjors maybe you have thoughts on this?
It would probably be good if we set default field values in the capnp file:
https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/ipc/capnp/mining.capnp#L40-L41
that match the c++ defaults:
https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/node/types.h#L40-L49
Note just directly changing defaults in the .capnp file would not be backwards compatible. It would cause problems if
...
(https://github.com/bitcoin/bitcoin/pull/33222#issuecomment-3211088504)
@Sjors maybe you have thoughts on this?
It would probably be good if we set default field values in the capnp file:
https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/ipc/capnp/mining.capnp#L40-L41
that match the c++ defaults:
https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/node/types.h#L40-L49
Note just directly changing defaults in the .capnp file would not be backwards compatible. It would cause problems if
...
💬 mzumsande commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#discussion_r2291461327)
> After the kill, we jump back exactly 2 blocks to `start_height`. I guess this is due to the clean shutdown in `restart_node()` on line 319 in the preceding test code above having fully committed that state to disk.
I think the previous `gettxoutsetinfo` (which flushes the chainstate) is the last time. But I added a restart, so that the added test doen't depend on the previous subtests. I also added a comment / did the rename.
(https://github.com/bitcoin/bitcoin/pull/33212#discussion_r2291461327)
> After the kill, we jump back exactly 2 blocks to `start_height`. I guess this is due to the clean shutdown in `restart_node()` on line 319 in the preceding test code above having fully committed that state to disk.
I think the previous `gettxoutsetinfo` (which flushes the chainstate) is the last time. But I added a restart, so that the added test doen't depend on the previous subtests. I also added a comment / did the rename.
💬 mzumsande commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3211161834)
[4625b4b](https://github.com/bitcoin/bitcoin/commit/4625b4b8f772de1cd852003645c75bfa68c6735a) to [a602f6f](https://github.com/bitcoin/bitcoin/commit/a602f6fb7bf5f9e57299f4d6e246c82379fad8d2): small changes to the test
(https://github.com/bitcoin/bitcoin/pull/33212#issuecomment-3211161834)
[4625b4b](https://github.com/bitcoin/bitcoin/commit/4625b4b8f772de1cd852003645c75bfa68c6735a) to [a602f6f](https://github.com/bitcoin/bitcoin/commit/a602f6fb7bf5f9e57299f4d6e246c82379fad8d2): small changes to the test
💬 BenWestgate commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3211192805)
> This could still be implemented—even as part of this PR.
>
> The wallet generates the seed, stores it, and then derives the BIP32 master key by deterministically combining the seed with the user-provided passphrase. This approach allows users to back up the raw seed while still requiring the passphrase for wallet recovery.
>
> Or am I missing something ?
Not part of the BIP-0093 spec.
If a passphrase is used it becomes a nested 2 of 2(passphrase, codex32 secret) causing funds loss
...
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3211192805)
> This could still be implemented—even as part of this PR.
>
> The wallet generates the seed, stores it, and then derives the BIP32 master key by deterministically combining the seed with the user-provided passphrase. This approach allows users to back up the raw seed while still requiring the passphrase for wallet recovery.
>
> Or am I missing something ?
Not part of the BIP-0093 spec.
If a passphrase is used it becomes a nested 2 of 2(passphrase, codex32 secret) causing funds loss
...
💬 fanquake commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211193252)
You'll also need to add the `capnp` install instructions for Alpine and Arch in `build-unix.md`.
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211193252)
You'll also need to add the `capnp` install instructions for Alpine and Arch in `build-unix.md`.
💬 BenWestgate commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3211231715)
> It seems to me that codex32 only plays a role in (1). And so importing it should be done with `addhdkey`. Exporting could be done with `gethdkeys`.
It would be a usability and compatibility improvement to begin exporting `hdseed` in the codex32 secret format as:
1. Easier to type, write and speak than Base58.
2. Error correcting, hand-verifiable checksum
3. Easier to extract the payload seed bytes without needing base58 conversions.
The 4 character identifier when exporting `hdseed`
...
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3211231715)
> It seems to me that codex32 only plays a role in (1). And so importing it should be done with `addhdkey`. Exporting could be done with `gethdkeys`.
It would be a usability and compatibility improvement to begin exporting `hdseed` in the codex32 secret format as:
1. Easier to type, write and speak than Base58.
2. Error correcting, hand-verifiable checksum
3. Easier to extract the payload seed bytes without needing base58 conversions.
The 4 character identifier when exporting `hdseed`
...
💬 achow101 commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3211267309)
I generally disagree with the approach in this PR. It's doing a lot of refactoring that feels too much like refactoring for the sake of refactoring. I think this can be dropped from the milestone.
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3211267309)
I generally disagree with the approach in this PR. It's doing a lot of refactoring that feels too much like refactoring for the sake of refactoring. I think this can be dropped from the milestone.
💬 Sjors commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211285558)
Arch instruction taken from: https://capnproto.org/install.html
For Alpine I added `capnproto` and `capnproto-dev` since they both exist, but didn't test if the latter is needed.
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211285558)
Arch instruction taken from: https://capnproto.org/install.html
For Alpine I added `capnproto` and `capnproto-dev` since they both exist, but didn't test if the latter is needed.
💬 maflcko commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211316649)
> For Alpine I added `capnproto` and `capnproto-dev` since they both exist, but didn't test if the latter is needed.
dev installs the other: https://cirrus-ci.com/task/6500081345495040?logs=install#L0
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211316649)
> For Alpine I added `capnproto` and `capnproto-dev` since they both exist, but didn't test if the latter is needed.
dev installs the other: https://cirrus-ci.com/task/6500081345495040?logs=install#L0
💬 maflcko commented on pull request "IPC followups for PR 31802":
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211318865)
lgtm ACK de65c86572c5ca96e388a444aae9aa73bbd8860a
(https://github.com/bitcoin/bitcoin/pull/33233#issuecomment-3211318865)
lgtm ACK de65c86572c5ca96e388a444aae9aa73bbd8860a
🤔 janb84 reviewed a pull request: "IPC followups for PR 31802"
(https://github.com/bitcoin/bitcoin/pull/33233#pullrequestreview-3141476594)
re ACK de65c86572c5ca96e388a444aae9aa73bbd8860a
(https://github.com/bitcoin/bitcoin/pull/33233#pullrequestreview-3141476594)
re ACK de65c86572c5ca96e388a444aae9aa73bbd8860a