💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276382028)
> I am also thinking of adding a util::Messages{Result&&} helper.

That sounds like a worthwhile improvement of the ergonomics here.

💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276417417)
It strikes me as unfortunate that the move constructor cannot not move the failure values across different result value types, meaning the failure needs to be passed in as a separate argument. At the same time the user would be allowed to return a `{std::move(result), util::Error{Untranslated("str error")}`, potentially (and I am still not quite sure if that is what is actually happening here) without the user noticing that this will not move the failure value and instead initialize a `Monostate
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276418105)
Run clang-format on new code?

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276421094)
I guess the build is fast and not needed to be cached for now?

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276415678)
nit:
```suggestion
make -j$(nproc) -C build
```

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

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276438436)
clang-formatted everything using `src/.clang-format`.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276439295)
Yea, it's only two files, so very fast, especially compared to the runtime of clang-tidy itself.

💬 stickies-v commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1276442786)
Can be made quite a bit more DRY:
```suggestion
self.log.info(f"Test -acceptstalefeeestimates option is not supported on non-regtest chains")
for chain in ["main", "test", "signet"]:
with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
conf.write(f'chain={chain}\n')
conf.write('acceptstalefeeestimates=1\n')
self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={inc_conf_file_path}", "-allowignoredc
...

💬 stickies-v commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1276443940)
Would generally avoid unrelated style nits, you're not touching this line (or even file, for that matter) for anything else, so best to just leave as is.

💬 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