💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053019508)
> Q: is `hash` expected to be stable between runs - or is that not important here?

Oof [no](https://stackoverflow.com/a/16452347/30124796)! Switched to insecure & fast MD5.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053032382)
Taken.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052974062)
We will re-`raise` above for `None`, see: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2014298202

🤔 luke-jr requested changes to a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2782326633)
Won't this turn `/home/user/arbitrarywalletname.db` into `/home/user/user_%d.legacy.bak` ? Seems incorrect?

💬 luke-jr commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2053051831)
Maybe makes sense to move this to a header and reuse it everywhere else?

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053051864)
This was re-added in later pushes as I was able to reproduce it on Windows with 5- second timeouts, not just 0 timeouts as hypothesized above (https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2018664612). See 1f639efca5e71a0ff208415d94e408a74778d4db.

👋 hodlinator's pull request is ready for review: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660)

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2819614471)
Notable changes since [previous update](https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2779505028) (`git range-diff master 77776c5 ea3cc83`).
* Changed parent test process to run a node, and use it to measure startup-times. That measurement is then used to to cap how long we wait before timing out due to wrong port.
* Forward test framework arguments like `--tmpdir` and `--timeout-factor` to child processes (now with stable hashing).
* Dropped the change to avoid logging node
...

💬 luke-jr commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2819640126)
This looks like it uses JSON-RPC inside Capnproto? Why not just use straight Capnproto?

💬 ryanofsky commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2819690304)
I was under impression that the only wallet paths that could be loaded are:

https://github.com/bitcoin/bitcoin/blob/06439a14c884d7f81f331646ad361e88b3037a51/src/wallet/wallet.cpp#L2974-L2977

So it would not be possible to load a bare db outside -walletdir. And if the migration code was backing up a bare db file inside -walletdir like `arbitrarywalletname.db` it would just be backed up as `arbitrarywalletname.db_1223.bak`

💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2819701337)
> This looks like it uses JSON-RPC inside Capnproto? Why not just use straight Capnproto?

Yes it is skipping the HTTP part of the protocol but keeping the JSON part. The capnproto interface this exposes looks like:

```
interface Rpc $Proxy.wrap("interfaces::Rpc") {
executeRpc @0 (context :Proxy.Context, request :Text, uri :Text, user :Text) -> (result :Text);
}
```

where `request` and `response` above are JSON text strings with the expected JSON-RPC fields. The schema could be u
...

💬 furszy commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2819768601)
> 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?

I don't think anyone is planning to touch the legacy wallet code anymore, and the migration procedure no longer takes an hour either (which was the reason behind its creation). So I'd say it's no longer relevant as a benchmark.

Also,
...

💬 pattiscott973 commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2053150648)
Send

💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2819784017)
> So it would not be possible to load a bare db outside -walletdir.

Good point

> And if the migration code was backing up a bare db file inside -walletdir like `arbitrarywalletname.db` it would just be backed up as `arbitrarywalletname.db_1223.bak`

This PR currently breaks that. Although `test_direct_file` doesn't currently fail, adding a check for the wallet's name in the backup path does make it fail, e.g.

```diff
diff --git a/test/functional/wallet_migration.py b/test/functional/
...

🤔 shahsb reviewed a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318#pullrequestreview-2782621768)
Concept ACK

🤔 shahsb reviewed a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318#pullrequestreview-2782622019)
Approach ACK

💬 shahsb commented on pull request "Fix failing util_time_GetTime test on Windows":
(https://github.com/bitcoin/bitcoin/pull/32318#issuecomment-2819918130)
ACK https://github.com/bitcoin/bitcoin/pull/32318/commits/51e4c5b42260855e5b5306d677ad34560976b9fd

📝 leonarddt05 opened a pull request: "Update multiprocess.md"
(https://github.com/bitcoin/bitcoin/pull/32321)
Hi,

Two suggestions for this doc:

1. "the getBlockHash method of the generated Chain server subclass in bitcoin-wallet" Correction: This should be "in bitcoin-node", not "bitcoin-wallet", since the server class resides in the node process.

2. "capnp files in src/ipc/capnp/ was mostly done" Correction: "were mostly done" (subject-verb agreement: plural noun "files")

Thanks.

🤔 ajtowns reviewed a pull request: "miner: timelock the coinbase to the mined block's height"
(https://github.com/bitcoin/bitcoin/pull/32155#pullrequestreview-2782594487)
Approach ACK 895beb30b375c04804e9596ed798ab56730dcae1

💬 ajtowns commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2053224512)
Should these be `(height - 1) % LOCKTIME_THRESHOLD` ? Or are we just assuming a hard fork to a new transaction format in the next 9500 years?