💬 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
...

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1653188244)
> but it takes more than 2 hours to run

Yes, this is expected. Instead of just running `bitcoind` through qemu, now the *whole* container is run through qemu.

> unrelated errors

Jup, this should be unrelated, I presume it also happens on current master. You may be able to fix it by upgrading your `qemu-s390x --version`. IIRC qemu 7.2 or later is required.

> this is roughly the same (but simpler to configure from our side) than running an x86 image, manually installing qemu, and then
...

💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1653195617)
Thanks Marco, thats a very helpful explanation (and I nice changes for us!).

I will give it another go this afternoon after trying to update qemu.

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1653206694)
(reworked to second commit to only use a single `sed` command, and add documentation for why it is needed)

👍 TheCharlatan approved a pull request: "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)"
(https://github.com/bitcoin/bitcoin/pull/28092#pullrequestreview-1549403983)
ACK 08eb5f1b67e2af009549717eb5c66b7d7905731f

I'm fine with the warnings being printed until the mingw toolchain is rolled out everywhere.

Guix build:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
daeec3e7ea34ddf28200d95d96e3295cadfc48fba9d95234880
...