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

💬 fanquake commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653243314)
Updated to use the new plugin, https://github.com/theuni/bitcoin-tidy-plugin, and new check `bitcoin-unterminated-logprintf`.
Updated the PR description.

cc @theuni @stickies-v @dergoegge @TheCharlatan

👋 fanquake's pull request is ready for review: "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)

💬 dergoegge commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653248089)
`lint-logs.py` can probably be deleted?