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

...

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1668402066)
ACK 7f883b33bb89205a9d00c2d20d363a36a0167c7c

💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1668468059)
Rebased [`au.027`](https://github.com/jamesob/bitcoin/tree/au.027) -> [`au.029`](https://github.com/jamesob/bitcoin/tree/au.029)

The merge of #27607 really made that difficult.

This rebase fixes merge conflicts as well as incorporates @sdaftuar's changes from #27746. His net changes are in the first commit here.

I'm very tired of this project and hope we can focus effort on getting this changeset reviewed and merged to be done with the development of this feature.

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286334743)
Yea, the motivation here is just to 'speed up' some existing tests. But I like the idea of "I was expecting to see a test that makes an outbound connection to a node that misbehaves and doesn't get banned." this kind of coverage. I'm gonna add it.

💬 ryanofsky commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286352893)
In commit "rpc: Add Arg() default helper" (fa6b2da57ef7ff125a493c7835cb15935255ff8c)

Any reason to not just drop const from `HandleRequest` instead of making this variable mutable? Keeping the const there seems a little misleading

🚀 fanquake merged a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186)

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286378724)
You're right. I am gonna change it for tests that previously had set it for all nodes.

📝 Crypt-iQ opened a pull request: "net_processing: ensure mapBlockSource is removed from in ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/28235)
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the BlockChecked callback won't be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn't called, mapBlockSource won't be removed from. Fix that by always removing from mapBlockSource in ProcessBlock.

📝 Ayush170-Future opened a pull request: "fuzz: wallet, add target for Spend"
(https://github.com/bitcoin/bitcoin/pull/28236)
This PR adds fuzz coverage for `wallet/spend`.

Motivation: Issue [27272](https://github.com/bitcoin/bitcoin/issues/27272#issue-1628327906)

This PR adds Fuzz coverage to the whole concept of Creating a New Transaction as well as other sections of the Spend file. Because `CreateTransaction` is one of the most frequently used functions in the wallet codebase, merging this PR will significantly improve the wallet codebase's Fuzz testing!

I also used the `Singleton Class` concept for creati
...