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

🤔 TheCharlatan reviewed a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2449774883)
lgtm, but I don't think this should be prefixed with kernel: in both the title and the commit message. The point of having the notification interface is that the node code can extend kernel behaviour. The implementation details of that are not part of the kernel.

💬 TheCharlatan commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851029691)
Can't you skip the first conditional too then?

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851034724)
`IsArgSet` would be true when the user passes `-noversion`, so the `if`-block would be entered, which is intended to be avoided in this PR.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851054194)
I fail to see the typo, mind pointing it out?

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851065265)
Potentially yes, but I'm trying to keep the scope of the PR to the args I discovered here: https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013

Open to exploring it in a follow-up!

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

This is the same functional approach and error message already.
Creating a PSBT involves stripping the scriptSig and scriptWitnesses. Later, the hashes are compared. That's exactly what this change is doing too but the syntax is different. I can align the syntax to make it more clear it is doing the same thing.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851068380)
I prefer documenting the type.

💬 andremralves commented on issue "Functional tests: `feature_bind_port_discover.py` is failing":
(https://github.com/bitcoin/bitcoin/issues/31336#issuecomment-2489678021)
I followed the instructions in found here:
https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/test/functional/feature_bind_port_discover.py#L12-L21

After https://github.com/bitcoin/bitcoin/pull/29984 I think those instructions no longer apply.