💬 kevkevinpal commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2422155953)
I don't think we need the `what` variable if it's just used as a string to print directly after the if block
```suggestion
switch (broadcast_method) {
case node::ADD_TO_MEMPOOL_AND_BROADCAST_TO_ALL:
WalletLogPrintf("Submitting wtx %s to mempool and for broadcast to peers\n", wtx.GetHash().ToString());
break;
case node::ADD_TO_MEMPOOL_NO_BROADCAST:
WalletLogPrintf("Submitting wtx %s to mempool without broadcast\n", wtx.GetHash().ToString());
break;

...

💬 kevkevinpal commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2422147741)
Maybe I'm confused but why include this switch statement if previously we didnt need to? I don't see any if statement for `relay` here

💬 optout21 commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3392437781)
> * It looks to me like the `expected_code_path` assert is confirming that the `std::logic_error` exception throws at least once and that test coverage is complete.

I don't think it checks that exception is thrown at least once, but it checks that either there is no exception, or if there is, it is that specific exception. With this change there is an assert to make sure the exception is the one expected. So I think the test code change is correct, but I agree that the "that all codepaths res
...

💬 RandyMcMillan commented on pull request "rpcnestedtests.cpp:remove qtest-obsolete members":
(https://github.com/bitcoin-core/gui/pull/900#discussion_r2422197293)
note: https://github.com/bitcoin-core/gui/actions/runs/18416608653/job/52481652961

only failure - which suggests the tests aren't actually testing qt version <= 6.2

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2422282858)
I have already applied your previous suggestion and added you as coauthor.

💬 GaloisField2718 commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3392867370)
https://github.com/bitcoin/bitcoin/blob/v30.0/doc/release-notes.md

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3393048219)
> Should probably only be available if advanced coin control is enabled in settings, and maybe only on a menu even then (if it made sense to do it as a tab, it should be within the main window, not a new window).
>
>
>
> The SVG source for the new icon should also be included.

Thanks for the comments! I will check soon!

💬 itokichi7777 commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3393221753)
tACK0f1f02995b

💬 TheCharlatan commented on pull request "Bugfix: Correct first-run free space checks":
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-3393232862)
Concept ACK

🤔 TheCharlatan reviewed a pull request: "Policy: Report reason inputs are non standard"
(https://github.com/bitcoin/bitcoin/pull/29060#pullrequestreview-3327213980)
Concept ACK

There are also some drahtbot llm linter suggestions.

💬 TheCharlatan commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2422872962)
Since we're renaming, I would prefer `ValidateInputsStandardness` here. The `Has` prefix implies a bool return value to me.

💬 TheCharlatan commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2422900883)
I don't quite follow why we are invoking the solver here. What is this supposed to test?

💬 TheCharlatan commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2422904900)
Why is this tested here?

💬 TheCharlatan commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2422905182)
In commit 910849007151cb8868f703ed759ee31f630eaea7:

Nit: Typo in the commit message s/resin/reason/.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2422946155)
ha, that's awesome. The queue's `std::packaged_task<void()>` change broke my mind because the task has obviously a different return value..
I see the trick now, it seems the `packaged_task<R()>` is wrapped into another `packaged_task<void()>` which forwards the call internally. That's elegant.

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3393496902)
So I've made the changes.

**Specifically:**

1. I added the .svg file, like discussed.
2. I set the coins tab to be in the same window and not open a new one.
3. I set the coins tab to be visible only if "coin control features" are enabled in the advanced options.

**The way it works is:**

When coin control features are disabled, then the "Coins" tab will be hidden, like in this picture:

<img width="600" height="400" alt="new_coins_1" src="https://github.com/user-attachments/asse
...

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3393553555)
Thank you for considering the suggestions, I find the new layout a lot easier to follow.

I have left a few more nits, but I'm fine doing them in a follow-up.
I like these memory optimizations, my only concern is that we're likely doing a lot heavier copying because of the regular compactions (especially since most move constructors seem unused based on previous fuzzing reports). As mentioned, in a follow-up we can experiment with leaving a tiny buffer.

I have also added a few redundant as
...

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2422338701)
This method is called from two places, one of which is dropping the pre-cacululated cluster level that we could access through `FindClusterAndLevel` - the other call doesn't seem to, I guess that's why you decided to always recalculate.
It seems to me it wouldn't complicate much to provide it when it's available and recalculate when it's not, something like:
```patch
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
--- a/src/txgraph.cpp (revision 262762bbb6f7157ba8c54972909c9214448c304b)
+++ b/src
...

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2422371348)
nit: for consistency:
```suggestion
Assume(!GetTxCount());
```

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2422371564)
nit: likely missed because of the extra space
```suggestion
if (!GetTxCount()) return 0;
```