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

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286421558)
Fixed

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668641951)
Force-pushed:

1. Renamed `whitelist_peers` to `noban_tx_relay` and set up it `True` for tests that explicity was whitelisting peers with this purpose of speeding up. - https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1274667131
2. Added test coverage for an outbound node that misbehaves and doesn't get banned. - https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275440729
3. Updated `-blocksonly` documentation.
4. Changed `InitializePermissionFlags` to deal with `https://g
...

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286425941)
I think that `CreateSyncedWallet` will call `SetupDescriptorScriptPubKeyMans()` which internally will make a seed.
```cpp
void CWallet::SetupDescriptorScriptPubKeyMans()
{
AssertLockHeld(cs_wallet);

if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
// Make a seed
CKey seed_key;
seed_key.MakeNewKey(true);
CPubKey seed = seed_key.GetPubKey();
assert(seed_key.VerifyPubKey(seed));

// Get the extended key
CExtKey maste
...

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286426350)
s/chose/choose

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1286427458)
> This interface is not the right place for a callback like this because it is not a "net event".

Hmm ok, that would avoid locking `cs_main` in the open connections thread. Which is nice.

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286427767)
I think you could keep same pattern as you had previously:
```cpp
std::vector<int> random_positions = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, recipients_size), fuzzed_data_provider.ConsumeIntegral<int>()};
int change_pos = PickValue(fuzzed_data_provider, random_positions);
```

instead of
```cpp
int random_positions[3] = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, mtx.vout.size()), fuzzed_data_provider.ConsumeIntegral<int>()};
int change_pos = fuzzed_data_
...

💬 luke-jr commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1668653252)
>Simply claiming this doesn't seem helpful. What is the compile error you encountered? (Ideally with steps to reproduce)

The several issues bootstrapping Rust are well-known and documented elsewhere. Fixing it is not within the scope of this project. It simply shouldn't be relied on until then.

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668673685)
1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn't build