👋 achow101's pull request is ready for review: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124)
(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.
(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.
(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
(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
(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
...
(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)
(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?
(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?
(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?
💬 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.