💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1606611069)
I think the key point of both sentences is that all dependency versions must match exactly. Installing via distro packages or via pip are just two ways. Both can fail in the same way (outdated or too-recent dependency versions), so communicating them in the same section makes sense, but again, just a nit.

📝 hebasto opened a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/30143)
This PR updates the NetBSD Build Guide to reflect:
- the recent NetBSD Release
- GCC minimum supported version update (https://github.com/bitcoin/bitcoin/pull/28348 and https://github.com/bitcoin/bitcoin/pull/29091)
- Python minimum supported version update (https://github.com/bitcoin/bitcoin/pull/28211)

Also a smaller package set has been suggested:
- `boost-headers` instead of the full `boost`
- `qt5-qtbase qt5-qttools` instead of the full `qt5` (similar to https://github.com/bitcoin/b
...

💬 maflcko commented on pull request "doc: Update NetBSD Build Guide":
(https://github.com/bitcoin/bitcoin/pull/30143#issuecomment-2120337184)
utACK 85e480a41ad0901a8b46759e921c187ebbbcccdf

💬 dergoegge commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1606683947)
Structs that are part of the public interface should probably go into `txdownloadman.h`. Otherwise one needs to include the impl header, e.g. like `txdownloadman.h` does right now, which defeats the purpose of pimpling to some extend.

💬 dergoegge commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1606717505)
Iiuc correctly, clearing the filters here has been removed as we don't want `TxDownloadManager` to depend on `ChainstateManager`.

An alternative to the new synchronous validation interface callback could be to preserve the previous behavior (clearing the filters in `AlreadyHaveTx` on a tip change) by introducing an interface for TxDownloadMan to fetch the latest tip hash. For example, `TxDownloadOptions` could be extended to hold a lambda that returns the latest tip hash, which in production
...

👍 dergoegge approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2066132059)
Code review ACK 636e4864516782b7e6e33be2b2ec7743afaae8ba

💬 maflcko commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606723428)
nit: Now that this is merged, it could say "in 27.0 and prior releases." Otherwise, on 29.x it will read as-if 28.0 had it missing.

💬 maflcko commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606724711)
nit instead of duplicating the section from `doc/JSON-RPC-interface.md` here, it seems better to link/refer to it. Otherwise, if the section is updated, this may go stale or must be updated at the same time.

💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2120390457)
@theuni https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597108664 has been resolved by https://github.com/bitcoin/bitcoin/pull/30047/commits/d196442c015c4cbf490751a650ecb3aa29321442, if you have time for a second look/reACK

🤔 furszy reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2066175552)
utACK 4a2df05e

💬 furszy commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1606753156)
nit: should use `fs::PathToString(path)`

💬 maflcko commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1606762456)
> PathToString

That is only for internal use. However, the result here will be returned on RPC, so the dev-notes apply:

```
- Use `fs::path::u8string()`/`fs::path::utf8string()` and `fs::u8path()` functions when converting path
to JSON strings, not `fs::PathToString` and `fs::PathFromString`

- *Rationale*: JSON strings are Unicode strings, not byte strings, and
RFC8259 requires JSON to be encoded as UTF-8.

🤔 furszy reviewed a pull request: "validation: improve performance of CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/28339#pullrequestreview-2066200941)
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8

💬 ajtowns commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1606763359)
`assert_equal` is helpful because when `a == 3` fails, it can be helpful to know what the value of `a` actually is without having to add debug code and rerun. If `a != 3` fails, though, you already know that `a == 3`, so this doesn't seem very helpful.

There are cases where you're comparing `a != b` and may not be sure which ones getting the wrong value where this could perhaps be helpful, but those seem pretty rare.

Concept -0 for me.

💬 ajtowns commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1606772606)
This error message doesn't make sense when `*args` is non-empty: if you say `assert_not_equal(a, 1, 2)` and `a` is 1 or 2, it will report `1 == 1 == 2` or `2 == 1 == 2`.

💬 marcofleon commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#issuecomment-2120491389)
Got it, thanks @brunoerg. I'll redo and get back to you then. I also realized I was looking at the wrong line in the coverage report. I should probably be looking at `wallet/scriptpubkeyman.cpp` not `wallet/test/fuzz/scriptpubkeyman.cpp`.

💬 maflcko commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606802910)
Looks like this is unused?

👍 willcl-ark approved a pull request: "net: make the list of known message types a compile time constant"
(https://github.com/bitcoin/bitcoin/pull/29421#pullrequestreview-2066265255)
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef

Based on range diff from previous ACK, all looks good!
`range-diff 2e312ea...b3efb48`

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1606818901)
As discussed offline, I think this change means we're ignoring the GUI-based reindex from https://github.com/bitcoin/bitcoin/pull/30132/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR1609, both for these optional indices but also for the block index

💬 maflcko commented on pull request "wallet: Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#issuecomment-2120525642)
I don't understand this change. It looks like a "Back" button is added to the warning pop-up, but it will only take you back to enter a different password, not to disable the password selection in the create wallet dialog. Wouldn't it be better to add the "Back" button to the password dialog?

Also, wouldn't it be better if the text from the two warning pop-ups was added to the top of the password dialog? Otherwise, it seems odd to first ask for a password from the user, then print two warning
...