💬 achow101 commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1653841139)
ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da

✅ fanquake closed a pull request: "Drop macOS ForceActivation workaround"
(https://github.com/bitcoin-core/gui/pull/744)

🚀 achow101 merged a pull request: "refactor: consistently use ApplyArgsManOptions for PeerManager::Options"
(https://github.com/bitcoin/bitcoin/pull/28148)

📝 furszy opened a pull request: "net: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170)
Derived from #28120 discussion.

By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.

The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections a
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276473986)
Just for reference, this isn't actually safe, since Bitcoin Core has multiple thread, which may log and interleave at will. But that is a pre-existing bug, and I presume this check just accommodates this, which is fine.

👍 MarcoFalke approved a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1550164036)
lgtm ACK

📝 pinheadmz opened a pull request: "test: blockstore test with chattr instead of chmod"
(https://github.com/bitcoin/bitcoin/pull/28171)
alternative to https://github.com/bitcoin/bitcoin/pull/27850

see https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1650601548

just testing ci for now

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