🤔 fjahr reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2525710496)
Apparently I started to review and got pinged now because of the rebase message. Leaving these nits here in case you want to address them in the rebase. Will look at this again soon.

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1899620252)
nit: Typo in 7c614b15bc31a7d6f7daaa48c046eb2c7cca697e description “or now”

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1899619894)
nit: Could add a comment here as well, something like

``` suggestion
// Only single key descriptors are allowed to be imported into a legacy wallet's keypool
bool can_keypool = parsed_descs.at(0)->IsSingleKey();
```

💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1923704181)
> Skip rescan if we provide rescan option as false (default true) so irrespective timestamp it won't scan block chain.

This seems confusing to users to silently ignore the timestamp. Why not simply have another timestamp value?

🤔 sipa reviewed a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2564587257)
@DrahtBot apparently wants this as a review?

ACK b7f3b6fd96ca2d5d1f9538affef682f778bd07e8

💬 maflcko commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2604711700)
> @DrahtBot apparently wants this as a review?

Any comment should work, but sometimes GitHub is down and the bot is missing a notification. Also, it is not possible to unrequest a review without triggering other GitHub bugs, so it is currently disabled: https://github.com/maflcko/DrahtBot/blob/8bde72c4821013c4a91753805b7b1be4af00513e/webhook_features/src/features/summary_comment.rs#L357

💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1923716358)
Optimizing for one specific block is also skewing towards something, just a bit different. Maybe the test should generate a number of blocks with some randomization of the txs and inputs and outputs instead.

💬 maflcko commented on pull request "test: Check that reindex with prune wipes blk files":
(https://github.com/bitcoin/bitcoin/pull/31696#issuecomment-2604723753)
> ACK [fa975d1](https://github.com/bitcoin/bitcoin/commit/fa975d1075668e3dc3366122463d6900e6d94d0c)

Looks like this was a stale commit?

💬 maflcko commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2604737698)
(removed label, because a pull exists)

💬 TheCharlatan commented on pull request "test: Check that reindex with prune wipes blk files":
(https://github.com/bitcoin/bitcoin/pull/31696#issuecomment-2604789168)
> Looks like this was a stale commit?

Fixed.

👍 laanwj approved a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2564690811)
Code review ACK 6eb94b3082e6ff6882e27ee98abb3c79f34abbe0

💬 hebasto commented on pull request "depends: Override default build type for `libevent`":
(https://github.com/bitcoin/bitcoin/pull/31661#issuecomment-2604792373)
My Guix build:
```
aarch64
22e26035bc1c8484369d855033af86a1209103e9f5c013db27c9a5e88f798de4 guix-build-d44626a9c2eb/output/aarch64-linux-gnu/SHA256SUMS.part
161a7f07ec0bbd8471453249319f179b6478f18b506b837404d6fd661c3790cf guix-build-d44626a9c2eb/output/aarch64-linux-gnu/bitcoin-d44626a9c2eb-aarch64-linux-gnu-debug.tar.gz
6288a7dfb39d4816b8e43666b7690db2a7fbbcc9077a4b1ec72ecac9804dd8a6 guix-build-d44626a9c2eb/output/aarch64-linux-gnu/bitcoin-d44626a9c2eb-aarch64-linux-gnu.tar.gz
428e0bf1
...

💬 brunoerg commented on issue "Inconsistent hardened derivation marker in `listdescriptors` output":
(https://github.com/bitcoin/bitcoin/issues/31694#issuecomment-2604807583)
> 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.

I think this is due `ToNormalizedString` called by `listdescriptors`:
```cpp
bool ToNormalizedString(const SigningProvider& arg, std::string& ret, const DescriptorCache* cache) const override
{
std::string sub;
if (!m_provider->ToNormalizedString(arg, sub, cache)) return false;
// If m_provider is a BIP32
...

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#discussion_r1923777999)
Agree, the nested loop here isn't necessary. It's already looping.

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2604816649)
> and have not tested it.

FWIW, i have tested this on a windows machine. Though a virtual one on amazon EC.

📝 l0rinc opened a pull request: "test,bench: validate `CheckTransaction`'s duplicate input detection in broader context"
(https://github.com/bitcoin/bitcoin/pull/31699)
This PR follows up on [#31682](https://github.com/bitcoin/bitcoin/pull/31682), isolating its testing and benchmarking improvements to focus on CheckTransaction's duplicate input detection and performance in realistic contexts, avoiding the controversial consensus code change.

First commit adds tests to validate `CheckTransaction`'s ordering-based duplicate detection against the hash-based methods used elsewhere in the codebase.

Second commit replaces overly-specific microbenchmarks with `P
...

💬 fanquake commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2604834248)
Inconsistency in the binaries leading to more workarounds/disabling test code: https://github.com/bitcoin/bitcoin/pull/31410.

⚠️ maflcko opened an issue: "intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/issues/31700)
Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.

I think it would be better to use mocktime to halt the test when needed, so that it can even pass when the delay is zero (or close to it).

I haven't looked in detail whether this is the same, but there was one instance in the CI: (Possibly the getdata was split into two, so the check didn't pick it up, because it assume
...

⚠️ maflcko typed an issue: "intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/issues/31700)
Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.

I think it would be better to use mocktime to halt the test when needed, so that it can even pass when the delay is zero (or close to it).

I haven't looked in detail whether this is the same, but there was one instance in the CI: (Possibly the getdata was split into two, so the check didn't pick it up, because it assume
...

💬 l0rinc commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2604837850)
@1440000bytes, this behavior isn't helpful - you're just reiterating what I've already explained, without suggesting workable solutions.

Anyway, I've created an [alternative PR](https://github.com/bitcoin/bitcoin/pull/31699) that focuses solely on the tests and benchmarks, excluding the controversial optimizations. It validates the affected code and removes microbenchmarks that overemphasize this segment's significance, replacing it with a higher-level one, as suggested by @mzumsande.