🚀 glozow merged a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243)

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052869871)
I might be missing something, but it would have to be a span of `CTxUndo`, and you'd have to move `WriteBlockUndo` to `ConnectTip` too, no? I wasn't quite sure about doing that change here though. The implications of separately writing the undo data and raising the `BlockIndex` validity level are not quite clear to me yet. There is always a question where to draw the scope line with these refactors. While it might not be useful to write undo data if we ever get to the alternative kernel API clie
...

💬 darosior commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052906047)
Oh, i was just reading the diff and overlooked `WriteBlockUndo`! Yes that makes sense.

🤔 l0rinc reviewed a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2782051429)
I also vote for squashing the last commit.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052883498)
https://docs.python.org/3/library/subprocess.html#subprocess.TimeoutExpired.output claims:
> This is always [bytes](https://docs.python.org/3/library/stdtypes.html#bytes) when any output was captured

Can we simplify to:

```suggestion
child_output = e.output.decode("utf-8") if e.output else ""
```

?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052899485)
I'm not a fan of a incomplete iteration, can we separate the generator expression to the first line, something like:
```suggestion
class_name = next((arg[16:] for arg in sys.argv if arg.startswith('--internal_test=')), None)
```
or use a regexp to deduplicate the match and substring (untested):
```suggestion
class_name = next((m[1] for arg in sys.argv if (m := re.match(r'--internal_test=(.*)', arg))), None)
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052915468)
do we need regular expression matching here or can we use `output.count("Traceback")` as well?
```suggestion
if (n := output.count("Traceback")) != 1:
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052891901)
nit: `assert False` is a bit hacky in my opinion - can we use the same style that we did on line 60?
```suggestion
print("Should have failed earlier during startup.")
raise
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052912020)
If I understand this correctly, we may want to preserve the seed properties - and xor preserves the distribution properties better than addition:
```suggestion
args.append(f"--randomseed={self.options.randomseed ^ hash(expected_exception)}")
```

Q: is `hash` expected to be stable between runs - or is that not important here?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052904553)
Based on the checks we're doing on `e` at the top, `error_num` can potentially be `None` here - is that expected?

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2819380673)
I have added a commit to also set nLocktime / nSequence for coinbase transactions created in fuzz targets.

> The churn in the first commit is pretty annoying.

@Sjors (from this and in-person discussions) i have split as much as possible of the changes from this commit into preparatory commits. Now there is in the main commit only the necessary churn to keep commits hygienic. Hopefully it helps.

👋 achow101's pull request is ready for review: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124)

💬 achow101 commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-2819389756)
Apparently migrating the legacy wallet to descriptors is also a way to fix this issue.

💬 achow101 commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2819392003)
Removed the fix commit since we are expecting legacy wallets to go away soon, and migration is a valid way to fix this problem. The remaining test commit is updated to work without the automatic repair being logged. It additionally is changed to check that no `keymeta` records exist after the migration.

💬 achow101 commented on pull request "rpc: Allow fullrbf fee bump in (psbt)bumpfee":
(https://github.com/bitcoin/bitcoin/pull/31953#issuecomment-2819404698)
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751

🤔 glozow reviewed a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953#pullrequestreview-2782156380)
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2819431273)
Rebased 8892cb50db63656991e4d411ab804a9535f991a0 -> 81c0b9edfe533afbb2f4dda56142afdedffdb347 ([`pr/wrap.28`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.28) -> [`pr/wrap.29`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.29), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.28-rebase..pr/wrap.29)) due to conflict with #31862. Also dropped check for `build/src` directory no longer needed after #31161

This PR is scheduled for a review club discussion in a few weeks
...

🚀 achow101 merged a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953)

💬 achow101 commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2819442462)
Is a benchmark that runs only once actually useful?

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2819455310)
Not sure, we can decide to delete it as well - I'll push tomorrow a fixed version based on @furszy's recommendation (but have to retest it first after my benchmarking servers free up again).
@furszy, is this still relevant or would deleting it be simpler?