💬 fjahr commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2819486409)
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
Just a note: If this test was run by CI we would need to flip the order of the commits because the first commit would fail without the second but the second would succeed without the first. There is a CI job that runs every commit individually. But since this test isn't executed by CI it doesn't matter here.
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2819486409)
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
Just a note: If this test was run by CI we would need to flip the order of the commits because the first commit would fail without the second but the second would succeed without the first. There is a CI job that runs every commit individually. But since this test isn't executed by CI it doesn't matter here.
💬 achow101 commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2819504455)
I guess it is useful as a spot check, but the accuracy is not very good.
Concept ACK
Tested on Windows and it worked. Was also able to reproduce failure 3, but don't know how to reproduce failure 2.
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2819504455)
I guess it is useful as a spot check, but the accuracy is not very good.
Concept ACK
Tested on Windows and it worked. Was also able to reproduce failure 3, but don't know how to reproduce failure 2.
💬 TheCharlatan commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052998450)
Ah, I see. I also tried to come up with something, but did not land on anything satisfying either. Was briefly thinking of adding an optional field in the `BlockCreateOptions`, but that would defeat the point.
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052998450)
Ah, I see. I also tried to come up with something, but did not land on anything satisfying either. Was briefly thinking of adding an optional field in the `BlockCreateOptions`, but that would defeat the point.
👍 TheCharlatan approved a pull request: "miner: timelock the coinbase to the mined block's height"
(https://github.com/bitcoin/bitcoin/pull/32155#pullrequestreview-2782260132)
ACK 895beb30b375c04804e9596ed798ab56730dcae1
(https://github.com/bitcoin/bitcoin/pull/32155#pullrequestreview-2782260132)
ACK 895beb30b375c04804e9596ed798ab56730dcae1
💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2819544950)
I think there's one more wrinkle to this issue: wallets outside of the wallets directory are probably stored where the wallet file is the name of the wallet, rather than the directory being the name, and containing a wallet.dat. An example of something similar to this is `test_direct_file` test case, although in that case the file is still in the wallets directory. In fact, I think this PR actually "breaks" that test case as the backup file is no longer named as we expect it (although the test d
...
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2819544950)
I think there's one more wrinkle to this issue: wallets outside of the wallets directory are probably stored where the wallet file is the name of the wallet, rather than the directory being the name, and containing a wallet.dat. An example of something similar to this is `test_direct_file` test case, although in that case the file is still in the wallets directory. In fact, I think this PR actually "breaks" that test case as the backup file is no longer named as we expect it (although the test d
...
👋 achow101's pull request is ready for review: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244)
(https://github.com/bitcoin/bitcoin/pull/31244)
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-2819555645)
All dependencies have been merged, ready for review.
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-2819555645)
All dependencies have been merged, ready for review.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053031058)
In this case real-world experience proved the documentation to be wrong. We do get a string on Windows, at least under some conditions.
It does make sense to invert the condition similar to what you did though! To avoid trying `None.decode("utf-8")`.
`None` serializes okay (to `"None"`) so we can return `e.output` for both the `str` and `None` cases.
```suggestion
# e.output is returned as bytes on Linux and str on Windows.
child_output = e.output.decode("utf-8") i
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053031058)
In this case real-world experience proved the documentation to be wrong. We do get a string on Windows, at least under some conditions.
It does make sense to invert the condition similar to what you did though! To avoid trying `None.decode("utf-8")`.
`None` serializes okay (to `"None"`) so we can return `e.output` for both the `str` and `None` cases.
```suggestion
# e.output is returned as bytes on Linux and str on Windows.
child_output = e.output.decode("utf-8") i
...
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052969592)
`raise` re-raises the currently caught exception. Think it's roughly equivalent to `throw;` in C++. One can do:
```python
raise AssertionError("...")
```
But I prefer using `assert` for new code. Considered raising `RuntimeError`, but this really is a code invariant, mostly for documenting, but also for catching changed behavior.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2052969592)
`raise` re-raises the currently caught exception. Think it's roughly equivalent to `throw;` in C++. One can do:
```python
raise AssertionError("...")
```
But I prefer using `assert` for new code. Considered raising `RuntimeError`, but this really is a code invariant, mostly for documenting, but also for catching changed behavior.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053020028)
Thanks! Took regexp form.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053020028)
Thanks! Took regexp form.
💬 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.
(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.
(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
(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?
(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?
(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.
(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)
(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
...
(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?
(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`
(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`