💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1834363955)
Hi, I'm not sure if I understood you correctly, but I guess you say it because it's possible to call the function with ``hindex = nullptr``.

To solve that I propose this changes:

```c++
double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex* pindex, const CBlockIndex* hindex) {
if (pindex == nullptr || hindex == nullptr)
return 0.0;
...
int64_t end_of_chain_timestamp;

if (hindex->nChainWork > pindex->nChainWork) {
int64_t header_age
...

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1834375774)
I'm relying on this comment from unmerged #16545:
https://github.com/bitcoin/bitcoin/blob/b5ef85497436c3e9e60c760465d8991592efef07/src/common/args.h#L112-L114

One could imagine a user having a config file containing:
```
version=1
<...many lines...>
noversion=1
```
The last occurrence of a bool option takes precedence. Agree it feels very contrived for this specific setting, but I'm trying to stick to the general principle of trying to allow negation if possible.

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1834376280)
Done!

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1834366452)
Thanks! Applied.

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834309312)
Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for `chainStateFlushed`, the `chainStateFlushed` implementation could just do that internally.

More generally I have been wondering if it would be a good idea to eventually strongly type these `Data` blobs (and similarly the enums)? For example here, wrapping it in a `BlockLocator` type. I don't think this is relevant as long as libmultiprocess is used to create the c++
...

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834348410)
Nit: Shouldn't this be called `txid`?

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834365825)
Why is the `role` typed as `UInt32` when the other enums are typed as `Int32`?

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834376651)
Should this include the version as well at this point?

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834362068)
How does this `write` argument here map to the `action` argument in the c++ interface? They seem to have different types.

👍 BrandonOdiwuor approved a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2423781623)
Code Review ACK af4d23178b420f46196fbace2176ce1fe94ed9cd

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2464747471)
@hebasto could you give this another look?

💬 dergoegge commented on pull request "doc: recitfy typos":
(https://github.com/bitcoin/bitcoin/pull/31253#issuecomment-2464751924)
Same as #30259?

💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1834398930)
> Hi, I'm not sure if I understood you correctly, but I guess you say it because it's possible to call the function with `hindex = nullptr`.

I am asking why it needs to be passed at all. Shouldn't it be the same for all callers?

💬 fanquake commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1834418655)
I've just shortened the comment here.

💬 maflcko commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2464789937)
I agree that the changes here are still useful. It would be good to rebase on top of https://github.com/bitcoin/bitcoin/pull/31174, maybe after it is merged.

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1834442999)
Oh, understood, yes, I think so.
But it's the way to not move the function inside ``chainman`` and keep the actual structure. Do you think is more reasonable move it inside?

👍 hodlinator approved a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2423920012)
ACK ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba

Implemented my suggestions (except comment removal suggestion) + broke out `parse_size()` since my last review.

*util_string_tests* tests passed locally.

Left one comment, but nothing blocking.

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834470480)
sdfsdfsd

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834472950)
As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.
```suggestion
// Length and type in "[flags][width][.precision][length]type"
// is not checked. Parsing continues with the next '%'.
```

💬 darosior commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1834499475)
This is not necessary anymore.