💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2494334027)
Took @glozow first two commits with a couple additional sanity check lines.

💬 theStack commented on pull request "test: avoid internet traffic in rpc_net.py":
(https://github.com/bitcoin/bitcoin/pull/31343#discussion_r1854360802)
> What about using -connect=0 or (edit: this will not stop addnode) -proxy=127.0.0.1:1 for this particular test?

I agree that the -proxy approach (as done also in #31142) is much better and it seems to work as expected. Note that I added the parameter not only for the problematic sub-test, but for the whole functional test, in order to prevent also other sub-tests from connecting outside.

💬 theStack commented on pull request "test: avoid internet traffic in rpc_net.py":
(https://github.com/bitcoin/bitcoin/pull/31343#issuecomment-2494401283)
Thanks for reviewing @vasild! Force-pushed with your -proxy suggestion in https://github.com/bitcoin/bitcoin/pull/31343#discussion_r1853553795 and updated the PR description accordingly with test instructions.

💬 kevkevinpal commented on pull request "test: locking -testdatadir when not specified and then deleting lock and dir at end of test":
(https://github.com/bitcoin/bitcoin/pull/31328#issuecomment-2494403467)
> Code review [8963bc8](https://github.com/bitcoin/bitcoin/commit/8963bc860f6f55462851fbda28627b4483ba8240)
>
> Think this PR might be overly paranoid.
>
> Regardless of whether `G_TEST_GET_FULL_NAME` returns a value for the given test, is locking really necessary if directories are sufficiently unique (random) thanks to [faaaf59](https://github.com/bitcoin/bitcoin/commit/faaaf59f71ede057b2c1d369ef8db973c2f2dbc2) being included in the already merged #31317?
>
> ```c++
> const auto rand
...

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2494412531)
Tests passing, ready for re-review. I think we're close on this one.

💬 polespinasa commented on pull request "rpc: print P2WSH witScript in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r1854468502)
code rewritten based on this comment

📝 jonatack opened a pull request: "rpc, cli: add getbalances#total, and use it for -getinfo"
(https://github.com/bitcoin/bitcoin/pull/31353)
Add a "total" field in RPC getbalances to be able to easily see the total amount held in the wallet, and use that field for the wallet balances in CLI -getinfo.

Currently -getinfo only returns getbalances#mine.trusted for wallet balances. It would make sense to instead return the total balance. For instance, to see:

- watchonly balances
- coins returned to a wallet from an exchange or third party service that uses a fixed (reused) address, whether `avoid_reuse` is set or not, as it can b
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1854528651)
If #31346 lands first, `notifications().m_tip_block` could be an `Assert` and the comment removed, see https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493956840

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1854660293)
Long story short: I would find it suspicious if the test of a specific fix passes without the fix.
The cleanest way I found to make sure this isn't the case is to add the test commit before the fix commit and make sure CI fails if the test passes without the fix (see mentioned Spock [PendingFeature](https://github.com/spockframework/spock/blob/master/spock-core/src/main/java/spock/lang/PendingFeature.java#L13-L17)).
But since we don't have that here, I would prefer having the test in the same
...

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1854669075)
I'm a bit apprehensive on moving system-related code into the options. It just makes it a bit more complicated to patch out system-related code if we ever choose to ship support for the kernel library on bare metal. It is also why moved `system.h` into `common/` in the first place.

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1854672764)
Yeah, I thought of different approaches here too, happy to take an alternative here if somebody comes up with something. Otherwise we'll handle this once we get more queues, or a unified thread pool.

🤔 glozow reviewed a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096#pullrequestreview-2455948349)
> edit: Turns out it doesn't correctly mark package as failed if single tx fails.

Sorry about that! I'm happy with the `package.size() == 1` gate for the retry logic. Nice that the fuzzer caught that. Can add a low fee test case to package_single_tx to hit it I think.

ACK 278cb41e8785a4948972a4b4f635c2654edd0e3e

💬 glozow commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1854880571)
fwiw I'm not too concerned about this "encouraging" miners to patch. Miners might mine dust and this doesn't stop them from doing anything, but I don't think this would make that more likely given "subverting" it would only require `-dustrelayfee=0` and has always been possible. I think it's worth discussing and asking miners. But that's my thinking.

💬 glozow commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2495097900)
cc @stickies-v, many of your comments

🤔 ryanofsky reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2455631901)
Rebased c42d27d8c844b721b5aff384b3d014b1e0cc4783 -> 27083ecbc221095585cfff3bb208850e564723fd ([`pr/scripty.8`](https://github.com/ryanofsky/bitcoin/commits/pr/scripty.8) -> [`pr/scripty.9`](https://github.com/ryanofsky/bitcoin/commits/pr/scripty.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/scripty.8-rebase..pr/scripty.9)) to fix conflict with #31317 including suggested changes and some changes to the scripted diff to cover more arguments.

re: https://github.com/bitcoin/bitcoi
...

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854755963)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850670602

Thanks, fixed

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854659942)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850595520

> Is `isFalse()` equivalent to the former `IsArgNegated()` use?

Yes exactly,, if you look at the definition of IsArgNegated() it is just returning `isFalse()`:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/src/common/args.cpp#L454

This works because of the translation of negated values to `false` in `InterpretValue`:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7
...

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854758000)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850692509

> How come we keep the default value in one of the cases here but not the other, both are using .GetBoolArg in the original?

The scripted diff will only assign default values to Setting types if the same default value is used every place a setting is retrieved.

In this case the `-dnsseed` setting is retrieved 5 different places, and 4 of the places look like `args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED)` where `DEF
...

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854656393)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850502858

> What is `StringLiteral(std::nullptr_t) ->`?

This is a template deduction guide used to be able to pass nullptr_t as a string literal (used for a few settings that are only retrieved and never registered). I could explain more but chatgpt is much more great at this type of question: https://chatgpt.com/share/6740f058-e900-800a-afdf-7b56760db068

💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1854744728)
re: https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850653892

> In [b51a156](https://github.com/bitcoin/bitcoin/commit/b51a156f388926175afdbc50c2d563db404e3b81): I've learned to accept `static constexpr` (+`inline`...) in headers, but these other non-`constexpr` statics seem like they end up being "file-local" static vars in _every compilation unit_ that includes them. Seems off?

Good catch. I wanted to avoid changing these to keep the commit move-only as much as possible but it
...