👍 jamesob approved a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1550293430)
reACK 1c7582e

Based on the interdiff (https://github.com/jamesob/bitcoin-review-data/blob/master/28008.sipa.bip324_ciphersuite/4.1c7582e/interdiff.3.180909f.diff), which contains the few minor changes @sipa has mentioned above (`const` declarations, doc adds, small refactors).

> by extending my own hacky Python implementation using PyCryptodome (for ChaCha20, ChaCha20Poly1305, SHA256 and HKDF with HMAC_SHA256) + ellswift from our test framework for (see updated gist https://gist.github.com
...

🤔 MarcoFalke reviewed a pull request: "test, rpc: invalid sighashtype coverage"
(https://github.com/bitcoin/bitcoin/pull/28166#pullrequestreview-1550306892)
lgtm ACK 90c8f79e945863f3818748b86572948d1558aec3 🎥

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 90c8f79e945863f3
...

💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-1653987566)
https://github.com/lightning/bolts/pull/1096#discussion_r1276546764

Just noting that some LN spec work is touching these limits

💬 hebasto commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1653998189)
Closing in the light of https://github.com/bitcoin/bitcoin/issues/28098.

✅ hebasto closed a pull request: "ci: Use docker image cache for "Win64 native [vs2022]" task"
(https://github.com/bitcoin/bitcoin/pull/27771)

💬 dunxen commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1654038512)
Concept ACK

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654067926)
> I've just tried doing that locally and the test seems to have worked. The error shows up in the log as expected. Maybe it's just an issue with the larger test that it was part of at the time of that commit?

see #28171 rebased on master without any changes to CI framework. Using `chattr +i` does not prevent the root user from opening an "immutable" file in write mode [in this task](https://cirrus-ci.com/task/5758433270431744?logs=ci#L5258) and some other permission issue [in this one](https:
...

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276601095)
As you said, this one isn't a big deal but it does highlight the general issue of exceptions.

Generally I see a few ways to handle them:
- Change the code to be unambiguous. In this case it'd be something like adding a `LogPrintf_nonewline()` and using as necessary
- Hard-code exceptions in the plugin. This is basically how the current linter works.
- Try to find a preprocessor guard that applies only to clang-tidy. I can't find one, but I assume I'm just missing it.
- ^^, but define our
...

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654072882)
The first issue is inside a container:

```
chattr: Operation not permitted while setting flags on /tmp/cirrus-build-3580104950/ci/scratch/test_runner/test_runner_₿_🏃_20230727_161727/feature_reindex_readonly_206/node0/regtest/blocks/blk00000.dat
```

The second issue is that the file can't be deleted, because you'll have to grant/undo the permission again before the end of the test.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654074150)
https://duckduckgo.com/?q=chattr+inside+docker should fix it

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276607112)
You could just add a `/* TIDY IGNORE LOG */` (or similar comment) in the source code and then skip those with the plugin?

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276610630)

> * Try to find a preprocessor guard that applies only to clang-tidy. I can't find one
> but I assume I'm just missing it.

Whoops, here it is: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics

Neat. So the exceptions (if it works as documented) can become:
```
LogPrintf("foo..."); // NOLINT(bitcoin-unterminated-logprintf)
```

Which is nice and self-documenting :)

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276618881)
Pushed a commit here which tests this: https://github.com/theuni/bitcoin-tidy-plugin/commit/cdc007022b6cd40320b7e74ab4cbb8e67a6d6e17

It works as expected.

💬 achow101 commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1654108231)
ACK 131314b62e899f95d1863083d303b489b3212b16

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276624433)
> You could just add a `/* TIDY IGNORE LOG */` (or similar comment) in the source code and then skip those with the plugin?

Just to address this as I agree it'd be a reasonable solution, there's (currently at least) no access to comments from clang-tidy plugins as they're not part of the AST.

🚀 achow101 merged a pull request: "Fuzz: a more efficient descriptor parsing target"
(https://github.com/bitcoin/bitcoin/pull/27888)

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276667231)
Sorry for all the spam here, one more.

Given the nice opt-out above, I've pushed changes to my repo which eliminate all exceptions, so the plugin rule is now very simply: "every logprintf format string must end in a newline". That will mean we need to opt-out of the current "..." uses with NOLINT, which I think is the correct thing to do.

💬 jonatack commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#discussion_r1276671966)
That would encompass many of the methods in `rpc/util`. Where would you suggest to put them?

💬 jonatack commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1654195359)
Rebased.

💬 luke-jr commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276717254)
Shouldn't we still execute the notification? While it's not possible for the client to confirm it worked, it should still execute...