💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1652836631)
Updated to remove some unnecessary constructor changes and misleading comments associated with them.

💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677257)
Why not drop this entirely? Seems like a bug specifying an IP won't disconnect all nodes connecting from that IP...

💬 luke-jr commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1275677709)
In fact, maybe this should be split into a bugfix commit before adding subnet support.

💬 luke-jr commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1652845994)
>Rework of -addrinfo CLI is done using getaddrmaninfo RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511.

If it's going to be used for this, it shouldn't be hidden/test-only...

💬 luke-jr commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1275685570)
Is there a reason we shouldn't just support this? Normally it is okay to pass switch options in any order.

💬 MarcoFalke commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275760519)
> It's false by default and it has been set up for tests that were already using it.

You are also setting it for tests (all nodes in the test) that only have it set on one node. Not sure about that.

👍 MarcoFalke approved a pull request: "test, rpc: invalid sighashtype coverage"
(https://github.com/bitcoin/bitcoin/pull/28166#pullrequestreview-1549053986)
nit: I think it is fine to squash all the sighhashtype commits into one. They all do the same thing, and splitting it just makes review work harder, because at a minimum for each commit there is additional overhead for reviewers to check that a commit doesn't add a backdoor and removes it in the next commit.

💬 MarcoFalke commented on pull request "test, rpc: invalid sighashtype coverage":
(https://github.com/bitcoin/bitcoin/pull/28166#discussion_r1275763518)
in 9dde725f45c227189335d8745955c688aacd99c4: I think it is fine if you rephrase the commit body in your own words. The comment you are referring to (and not link to) may very well be wrong.

💬 MarcoFalke commented on pull request "test: remove unused code in `wallet_fundrawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652961889)
lgtm ACK 108c6255bc670bbf2732f2b79f6c32c26e181208

💬 MarcoFalke commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1275772804)
I don't think this is deterministic, is it? You can set the key from the fuzz input buffer instead.

💬 MarcoFalke commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1275774667)
Seems impossible that this is ever hit in the happy case, since a fuzz engine can not predict the 160-bit hash of a key in the wallet. Wouldn't it be better to move this symbol outside this scope and occasionally set it to a valid hash?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1652971889)
I think you'll also have to reply to the review comment https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1650601548

💬 TheCharlatan commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#issuecomment-1653046332)
Updated cc266f9ccd669866ee3121c8c03e08cd836ffba6 -> 5f38408d3a7def5286cc13dff95a49a9e2c8ea58 ([kernelRmUnivalueFollowup_0](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalueFollowup_0) -> [kernelRmUnivalueFollowup_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalueFollowup_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalueFollowup_0..kernelRmUnivalueFollowup_1))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/281
...

💬 stickies-v commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1275842550)
I think you forgot to remove this?

👋 TheCharlatan's pull request is ready for review: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866)

💬 TheCharlatan commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1275849009)
Whoops :(

💬 MarcoFalke commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#discussion_r1273594794)
same

💬 MarcoFalke commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#discussion_r1273594472)
```suggestion
SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {});
```

Nit: I think you can just let the compiler fill in the zeros for you by using `{}`.

💬 MarcoFalke commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1653094816)
From CI:

```
test/functional/mempool_datacarrier.py:31: error: Bracketed expression "[...]" is not valid as a type [valid-type]
test/functional/mempool_datacarrier.py:31: note: Did you mean "List[...]"?
Found 1 error in 1 file (checked 269 source files)
^---- failure generated from lint-python.py

💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1653142279)
I don't think I can give this much competent review I'm afraid. Partly because I can't get it working locally, and partly because I don't feel like I understand the changes fully yet.

I did try to run `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh`, but it takes more than 2 hours to run (amd64 local arch), and I keep getting (possibly) unrelated errors like this: [390x_build_error.txt](https://github.com/bitcoin/bitcoin/files/12180726/390x_build_error.t
...