💬 l0rinc commented on pull request "bench: fix used file that is not opened":
(https://github.com/bitcoin/bitcoin/pull/31693#discussion_r1922787576)
formatting is completely off - is this still a draft?

💬 rex4539 commented on pull request "bench: fix used file that is not opened":
(https://github.com/bitcoin/bitcoin/pull/31693#discussion_r1922792267)
Forgot to format before committing. Should be ok now :)

💬 l0rinc commented on pull request "bench: fix used file that is not opened":
(https://github.com/bitcoin/bitcoin/pull/31693#discussion_r1922793647)
Did you test it before committing?

💬 rex4539 commented on pull request "bench: fix used file that is not opened":
(https://github.com/bitcoin/bitcoin/pull/31693#discussion_r1922804895)
I compiled and ran `bench_bitcoin`.

💬 1440000bytes commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2603163348)
> I regularly got roughly ~1% speedup only. Not huge, but we have to decide if it's proportional to the risk or not.

> got a similar ~1% speedup (2 runs per bench, 19923 & 19933 seconds before, and 19681 and 19737 seconds after).

NACK

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2603194974)
Updated f3cfdf2b511776211a0399510ced735e556d510d -> 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d ([blockmanDB_9](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_9) -> [blockmanDB_10](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB_10), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB_9..blockmanDB_10))

* Took @maflcko's [suggestion](https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2602793391), for splitting move-only changes into a separate commit.
*
...

💬 luke-jr commented on pull request "doc: Fix invalid txid in `gettransaction` RPC example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2603200712)
My suggestion was to pull a txid from the mempool at runtime.

💬 ariard commented on pull request "Add NODE_TXRELAY_V2.":
(https://github.com/bitcoin/bitcoin/pull/30837#issuecomment-2603225301)
@maflcko Thanks if this PR can be re-opened.

The more ample rational behind this work has been published here:https://bitcoinops.org/en/newsletters/2024/12/06/ and it was under embargo for a bunch of months, as we were considering the impact on lightning implementations. I’ll go back on this PR + #30572 soon (tm). The problem is serious and for now it’s more _conceptual review_ that is calling for than fixing a CI.

⚠️ jp1ac4 opened an issue: "Inconsistent hardened derivation marker in `listdescriptors` output"
(https://github.com/bitcoin/bitcoin/issues/31694)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

If I use `importdescriptors` to import a descriptor that uses only `'` as the hardened derivation marker, the corresponding output of `listdescriptors` sometimes uses a mix of `'` and `h`.

### Expected behaviour

I would expect the output for a given imported descriptor to either match my input or otherwise use only one of `'` and `h`.

I would also expect the behaviour to be consistent a
...

💬 l0rinc commented on pull request "doc: Fix invalid txid in `gettransaction` RPC example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2603230619)
That’s an interesting idea for the command line, though it seems a bit outside the current scope.
Do we currently have any dynamic examples like that?
Also, how would this approach address static RPC reference pages, such as https://developer.bitcoin.org/reference/rpc/gettransaction.html?

💬 sipa commented on pull request "doc: Fix invalid txid in `gettransaction` RPC example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2603232349)
@luke-jr That sounds like overkill for a simple example.

💬 ariard commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2603233560)
Concept ACK.

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1922878607)
Thanks, done. I did a copy paste from above (line 1177) which I guess should be changed also but not here.

💬 achow101 commented on pull request "descriptors: Try pubkeys of both parities when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1922879973)
`pos` is really only used by `BIP32PubkeyProvider` to indicate child derivation index. Since no other `PubkeyProvider` can do derivation, it is ignored for all of them.

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1922880639)
> nit: there seems to be a whitespace needed before expected_result in the first loop.

Thanks, done.

> please run a formatter over the code if not done already.

Do you mean `git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v` ?

I get an error after running, maybe my version fo clang-format is different.

```
Formatting src/wallet/test/coinselector_tests.cpp
/home/yancy/Git/bitcoin/src/.clang-format:46:33: error: invalid boolean
BreakBeforeConceptDeclarat
...

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2603299196)
Updated PR to address format fix request.

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1922894011)
Done

🤔 tdb3 reviewed a pull request: "doc: Amend notes on benchmarking"
(https://github.com/bitcoin/bitcoin/pull/31690#pullrequestreview-2563208382)
Concept ACK

Adding more context for newcomers adds value and can reduce churn.

💬 tdb3 commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922891375)
I also tend to lean toward a per-PR approach, but do see the value in providing insight so new contributors know to provide a more complete case for a PR. Maybe something more encouraging could accomplish a similar goal, e.g. `Supporting the case for a performance improvement with benchmark results is encouraged`.

💬 tdb3 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922898256)
Fair point. Either approach (the refactor, or deferring) sounds reasonable. In the big picture, the warnings for blocksdir and datadir add value and I don't think walletdir warning should hold up the PR.

I do wonder how common it actually is that someone (using macOS) is using exFAT for just the walletdir, and would therefore miss the warning. I would imagine most users would be using the default walletdir location, and could therefore see the warning as it pertains to datadir.