💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2819162272)
@furszy has uncovered that it seems like migrating legacy to descriptors also solves the problem. Since removal of the legacy wallet is close, I will look into this a bit further and probably turn this PR into just a test.
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2819162272)
@furszy has uncovered that it seems like migrating legacy to descriptors also solves the problem. Since removal of the legacy wallet is close, I will look into this a bit further and probably turn this PR into just a test.
📝 achow101 converted_to_draft a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124)
A bug in 0.21.x and 22.x resulted in some wallets storing invalid derivation paths that are the concatenation of two derivation paths. This corruption follows a rigid pattern and can be easily detected and repaired.
The bug that resulted in such wallets was fixed in #23304 but wallets that already ran into it would still have bad data stored. This PR should fix such wallets.
Fixes #29109
(https://github.com/bitcoin/bitcoin/pull/29124)
A bug in 0.21.x and 22.x resulted in some wallets storing invalid derivation paths that are the concatenation of two derivation paths. This corruption follows a rigid pattern and can be easily detected and repaired.
The bug that resulted in such wallets was fixed in #23304 but wallets that already ran into it would still have bad data stored. This PR should fix such wallets.
Fixes #29109
💬 instagibbs commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2819163619)
reACK e3d7533ac954516d87a09b230c2898f5c9b1e213
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2819163619)
reACK e3d7533ac954516d87a09b230c2898f5c9b1e213
👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2781932567)
Code review ACK 341b2a99e646246a9e4d3aa283b2b59442cfad27. Only change since last review is making various improvements to comments and rebasing.
Would be nice to update doc/dependencies.md here since #32293 is no longer doing that but not important.
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2781932567)
Code review ACK 341b2a99e646246a9e4d3aa283b2b59442cfad27. Only change since last review is making various improvements to comments and rebasing.
Would be nice to update doc/dependencies.md here since #32293 is no longer doing that but not important.
👍 ryanofsky approved a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2781952277)
Code review ACK 0ac19a98f329fe8952f394509a7a29775ab805b0
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2781952277)
Code review ACK 0ac19a98f329fe8952f394509a7a29775ab805b0
💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2052821662)
In commit "doc: warn that CheckBlock() underestimates sigops" (0ac19a98f329fe8952f394509a7a29775ab805b0)
Might be good to say why this check is useful if it's not a problem for it to be inaccurate.
Might also be good if PR description mentioned motivation for the change. This all looks ok but it's a bit confusing because it's not clear what the context is.
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2052821662)
In commit "doc: warn that CheckBlock() underestimates sigops" (0ac19a98f329fe8952f394509a7a29775ab805b0)
Might be good to say why this check is useful if it's not a problem for it to be inaccurate.
Might also be good if PR description mentioned motivation for the change. This all looks ok but it's a bit confusing because it's not clear what the context is.
💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2819200264)
Do people think i should change the value of the coinbase transaction used in the fuzz test here as well? I previously kept it along with the PR introducing validation for those fields, but since i change this in unit tests already it might make sense to do it here.
(https://github.com/bitcoin/bitcoin/pull/32155#issuecomment-2819200264)
Do people think i should change the value of the coinbase transaction used in the fuzz test here as well? I previously kept it along with the PR introducing validation for those fields, but since i change this in unit tests already it might make sense to do it here.
💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2819202269)
utACK e3d7533ac954516d87a09b230c2898f5c9b1e213
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2819202269)
utACK e3d7533ac954516d87a09b230c2898f5c9b1e213
🚀 glozow merged a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640)
(https://github.com/bitcoin/bitcoin/pull/31640)
🚀 glozow merged a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243)
(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
...
(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.
(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.
(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 ""
```
?
(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)
```
(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:
```
(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
```
(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?
(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?
(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.
(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.