💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2489321625)
reworked https://github.com/bitcoin/bitcoin/commit/03ec3f6230f440b264abe3aa42a57fdd29bd3ba3 commit message

> Deduplicate assert_mempool_contents()
Deferring to future work since that's a different test, I'll review it if someone opens a clenaup PR.

> Sanity check for assert_mempool_contents()

Good idea, added minimal diff

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1850828070)
I'm not sure that's the best though, since we do not mark a coin as not dirty. That is the default state.

What about
"marking the coin as dirty unless `set_dirty` is set to false"?

💬 mzumsande commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1850831460)
> err on the conservative side and not introduce potentially breaking behaviour

Generally speaking, I don't see reason why we should artificially restrict a user from submitting a previously block via RPC, when they could request that block via `getblockfrompeer`, and I can't of use cases it would break.

Maybe one way to move forward would be to not remove the duplicate check in this PR (but document why it exists) - and instead remove it in a follow-up PR, together with a release note ab
...

💬 instagibbs commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1850834724)
The only danger I can foresee is if someone was somehow relying on them not being able to be committed to disk again. Would there be any performance concerns with commiting+re-pruning or similar?

💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1850853460)
That sounds good to me :+1:

💬 TheCharlatan commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#issuecomment-2489571213)
Checked the patch against the original moreutils package, and reproduced the stripped-down package. I think grabbing the specific tools we need from these kind of packages is a good thing.

Guix builds (aarch64):
```
74a02707614d7fc7ed460148b37e90c126d09c2e9b2962765de14d267a3f22d3 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/SHA256SUMS.part
263f079da730485eb583995c1fc6304b7ca957a668fd5aac8f879bc59746dc67 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-lin
...

💬 1440000bytes commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2489581872)
Can this use the same approach and error message as `combinepsbt`?

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850502858)
In f9910935c880c782c2b1d35acd38e1e18e6b4ad8:
What is `StringLiteral(std::nullptr_t) ->`? I take it some kind of template specialization but haven't come across regular braces and arrows being used in this way before.

🤔 hodlinator reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2448864512)
High-level review of c42d27d8c844b721b5aff384b3d014b1e0cc4783

Review instructions are a nice introduction. Feels a bit risky having links to personal branch instead of specific commit, but it's good to keep it flexible until ACKs come in.

---


### Negation?

I'm curious how you foresee negation checks being done. Having worked on proper handling of negated args recently, to me it feels like user code should have to jump through a hoop on the way to the non-negated value, if an arg su
...

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850595520)
Is `isFalse()` equivalent to the former `IsArgNegated()` use?

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850981914)
In f9910935c880c782c2b1d35acd38e1e18e6b4ad8:
Better to document type? This style of using `auto` as function-arg-type when not necessary feels like the opposite of C++ concepts, potentially leading to downstream compile errors. But interested to hear reasoning behind it.
```suggestion
static void Register(ArgsManager& manager, auto&&... register_options)
{
internal::SettingRegister<T, options, help>(manager, summary, category, help_fn, default_fn, get_fn, register_options...
...

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850670602)
```suggestion
# Trigger an error if any module listed as being nontransitive is ever included
```

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850653892)
In 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?

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850701093)
Maybe add value-less negation?
```C++
BOOST_CHECK_EQUAL(SettingTest<std::optional<int>>{}.AddArg("-nos").Parse().Get(), 0);
```

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(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?

Seems to compile fine when switching `DnsseedSetting` from using `::HelpArgs<DEFAULT_DNSSEED>` to `::Default<DEFAULT_DNSSEED>` like the `ForcednsseedSetting`.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850996627)
```suggestion
T SettingGet(const ArgsManager& manager, std::string_view summary, auto default_fn, auto get_fn)
```

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1850906340)
In f33c0b1969ee4c1c36475c876f46d9085feee134:
WIP?

📝 hebasto opened a pull request: "build: Use `-ffile-prefix-map` in release builds only"
(https://github.com/bitcoin/bitcoin/pull/31337)
This PR follows up on https://github.com/bitcoin/bitcoin/pull/30811, which inadvertently [broke](https://issues.oss-fuzz.com/issues/379122777) OSS-Fuzz coverage builds due to the `-ffile-prefix-map` option also implying `-fprofile-prefix-map`.

With this PR, only the `-fdebug-prefix-map` and `-fmacro-prefix-map` options are applied only for non-release builds.

**Note for reviewers:** Please ensure that https://github.com/bitcoin/bitcoin/issues/30799 is not reintroduced.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851027198)
> Or is the `ArgsManager::DISALLOW_NEGATION` meant to discourage people from requesting that :D?

Indeed, although I agree with `-nocolor` potentially equaling `-color=never`, it is flagged as forbidden and therefore the process is stopped before reaching the quoted code. I'm a bit hesitant to overrule the prior intent of disallowing it.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1851032146)
Done.