💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1668122010)
rebased :smiling_face_with_tear:

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1668136794)
rebased

🤔 furszy reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1565687522)
> I don't think the scenario you describe is likely to happen since we consider ourselves to be in IBD if our tip is older than 24h (which corresponds to ~144 blocks).

Thats not entirely true, misses an important part. We consider in IBD if our tip is older than 24hs (or the custom max tip age) and we haven't completed IBD yet. Once IBD is completed, the node remains out of IBD for the entire software lifecycle.

> > Suppose the node has successfully synced the chain, but later experienced
...

💬 garlonicon commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668239375)
> Rational: there's lots of ways for people to publish data in the Bitcoin chain, and lots of data has been published.

When it comes to witness data, I can easily prune all of them by downgrading my node to be a non-witness node. How to achieve the same thing for large OP_RETURNs?

> There's no reason for us to place artificial limits on this particular method.

The reason is you will encourage people to downgrade the way they publish data. Witness limit is 4 MB for a reason: it is easy t
...

💬 theStack commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286143759)
as per developer notes, [assertions shouldn't have side-effects](https://github.com/bitcoin/bitcoin/blob/624333455a5745a7f184d0df531dc348d0ac48dd/doc/developer-notes.md?plain=1#L742), so the following would be preferred:
```suggestion
bool success{chainman.m_blockman.ReadBlockFromDisk(block, pos)};
assert(success);
```
(same for the second test below)

🚀 fanquake merged a pull request: "doc: remove Fedora libdb4-*-devel install docs"
(https://github.com/bitcoin/bitcoin/pull/28231)

💬 RandyMcMillan commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668341684)
Concept ACK



macOS Catalina Version 10.15.7 (19H2026)

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286218964)
Yea, I'm going to change it

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1285999628)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284653088

> [835f094](https://github.com/bitcoin/bitcoin/commit/835f09452be5449f67fa44ecfa8a178404bff956) It looks like this empty vector might be declarable `constexpr`, at least with clang 16.0.6 arm64. Possibly not uniformly until C++20.

This is a good idea, but I think you are right it requires c++20. With my compiler (clang 13) I see:

```c++
./util/result.h:85:38: error: constexpr variable cannot have non-literal type
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284708030)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284654294

> [9ec1bda](https://github.com/bitcoin/bitcoin/commit/9ec1bdaff1cba965772a210e1e8326fc4f010aee) Can `Base` here be private instead of protected?

It could, but in general I prefer to use `protected` over `private` when it is safe for extensibility and testing, and in this case I want to support adding a `ResultPtr` type like the one in #26022 which inherits from `Result`. Also, `Base` is just a type alias for a public
...

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1563338368)
Updated 9e80d0b754a28733c79a52c8e0431616c31d071c -> 7f883b33bb89205a9d00c2d20d363a36a0167c7c ([`pr/bresult2.39`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.39) -> [`pr/bresult2.40`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.40), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.39..pr/bresult2.40)) responding to new review comments and also making some internal changes within the PR to reduce unnecessary diffs between commits.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1286183719)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284664601

> ```
> ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
> ```

Thanks, added

📝 martinus converted_to_draft a pull request: "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile"
(https://github.com/bitcoin/bitcoin/pull/28226)
TLDR:
* Adds a `TRACE_RAII` macro to easily trace runtime of a code block.
* Switch to `CBufferedFile` in `BlockManager::ReadBlockFromDisk` is slightly faster:
* 9% faster unserialization => 1.2% faster `-reindex-chainstate`

---
While profiling `-reindex-changestate` I saw lots of `fread()` calls in in `BlockManager::ReadBlockFromDisk`. This replaces the use of `CAutoFile` with `CBufferedFile` with a small buffer, leading to much fewer calls to
`fread()`, which gives a little speedup.
...

👋 amitiuttarwar's pull request is ready for review: "doc: diversify network outbounds release note"
(https://github.com/bitcoin/bitcoin/pull/28189)

🤔 stickies-v reviewed a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1565750058)
Approach ACK fa6b2da57ef7ff125a493c7835cb15935255ff8c

This makes for a much more elegant interface throughout the RPC methods, with minimal overhead.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286137440)
I like how you changed this in your latest force push, can be marked as resolved

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286146628)
nit: While correct, I think this could benefit from a useability hint, e.g.:

```
* This function only works during m_fun(), i.e. it should only be used in RPC method implementations.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286133078)
Thanks, you're right. I always assumed they were instantiated along with `CRPCTable`, but as you say they're freshly created on every `CRPCTable::execute()` call because of [this line](https://github.com/bitcoin/bitcoin/blob/624333455a5745a7f184d0df531dc348d0ac48dd/src/rpc/server.h#L109d). Can be marked as resolved.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286262199)
Makes sense, thanks for the explanation. Resolved.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286256642)
This implementation only supports being called on parameters with a `RPCArg::Default` fallback. Do you envision this being extended to also support `RPCArg::DefaultHint` and `RPCArg::Optional::OMITTED` fallbacks, and if so, how?

I think one way is with a `std::optional<int> RPCHelpMan::Arg<std::optional<int>>(size_t i) const` specialization. I tinkered around with it a bit and this is what I came up with:

<details>
<summary>git diff on fa6b2da57ef7ff125a493c7835cb15935255ff8c</summary>

...