💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211303547)
In commit "refactor: prepare `DBWrapper` for obfuscation key change" (c678b5dd79c3ed68b5c2537fdbf3de485db09021)
Similar to my comment in the last commit it is not clear if this is a code cleanup or a necessary change and it would be helpful if commit message mentioned the intent of the changes or why they are necessary.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211303547)
In commit "refactor: prepare `DBWrapper` for obfuscation key change" (c678b5dd79c3ed68b5c2537fdbf3de485db09021)
Similar to my comment in the last commit it is not clear if this is a code cleanup or a necessary change and it would be helpful if commit message mentioned the intent of the changes or why they are necessary.
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211296943)
In commit "refactor: prepare `DBWrapper` for obfuscation key change" (c678b5dd79c3ed68b5c2537fdbf3de485db09021)
All these changes seem reasonable, but I don't understand which of these changes help "prepare" for the new key type. It would be nice if commit message said why existing code wasn't compatible with the new key type.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211296943)
In commit "refactor: prepare `DBWrapper` for obfuscation key change" (c678b5dd79c3ed68b5c2537fdbf3de485db09021)
All these changes seem reasonable, but I don't understand which of these changes help "prepare" for the new key type. It would be nice if commit message said why existing code wasn't compatible with the new key type.
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211552605)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211078238
Looks like the code changes in the latest push, but here was the test I was going to suggest that relied on public methods only and I think covered everything.
<details><summary>diff</summary>
<p>
```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -74,32 +74,24 @@ BOOST_AUTO_TEST_CASE(xor_bytes_reference)
}
}
-BOOST_AUTO_TEST_CASE(obfuscation_constructors)
-{
- using K
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211552605)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211078238
Looks like the code changes in the latest push, but here was the test I was going to suggest that relied on public methods only and I think covered everything.
<details><summary>diff</summary>
<p>
```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -74,32 +74,24 @@ BOOST_AUTO_TEST_CASE(xor_bytes_reference)
}
}
-BOOST_AUTO_TEST_CASE(obfuscation_constructors)
-{
- using K
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211549971)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211075810
I was going to suggest dropping the serialization methods since current code isn't really using them. But I like Marco's suggestion to use them more.
For reference, change I was going to suggest is below. But Marco's approach seems better if it works.
<details><summary>diff</summary>
<p>
```diff
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -262,11 +262,8 @@ CDBWrapper::CDBWrapper(const DBParams& params
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211549971)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211075810
I was going to suggest dropping the serialization methods since current code isn't really using them. But I like Marco's suggestion to use them more.
For reference, change I was going to suggest is below. But Marco's approach seems better if it works.
<details><summary>diff</summary>
<p>
```diff
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -262,11 +262,8 @@ CDBWrapper::CDBWrapper(const DBParams& params
...
💬 furszy commented on pull request "wallet: Set migrated wallet name only on success":
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2211636309)
Might worth pulling this into a separate function. We use the exact same code in the existing `test_failed_migration_cleanup()` and also #32273 includes it on `test_failed_migration_cleanup_relative_path()`.
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2211636309)
Might worth pulling this into a separate function. We use the exact same code in the existing `test_failed_migration_cleanup()` and also #32273 includes it on `test_failed_migration_cleanup_relative_path()`.
🤔 furszy reviewed a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3026939999)
ACK 3a0be655c4fd5c74731f1fcb57c45e6a8c3c362f with a small nuance.
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3026939999)
ACK 3a0be655c4fd5c74731f1fcb57c45e6a8c3c362f with a small nuance.
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026880589)
Split out a test commit, moved a rename to the scripted diff, clarified test refactors and testing strategies in commit messages.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026880589)
Split out a test commit, moved a rename to the scripted diff, clarified test refactors and testing strategies in commit messages.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211613705)
We can still do that, but given that we need to be able to display the contents of the obfuscation anyway (unless we go back to reading/writing vectors), I think the current way is probably simpler. Please let me know if you disagree.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211613705)
We can still do that, but given that we need to be able to display the contents of the obfuscation anyway (unless we go back to reading/writing vectors), I think the current way is probably simpler. Please let me know if you disagree.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211627396)
Many of those changes were [suggested during review](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837161349). `dbwrapper` was added because of
https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057969172, after I saw that IBD failed even after all tests were passing. I've extracted it a separate commit now and explained the changes - and moved a remaining rename to the later scripted diff.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211627396)
Many of those changes were [suggested during review](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837161349). `dbwrapper` was added because of
https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057969172, after I saw that IBD failed even after all tests were passing. I've extracted it a separate commit now and explained the changes - and moved a remaining rename to the later scripted diff.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211610091)
Do you recommend we apply any of that after the recent changes?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211610091)
Do you recommend we apply any of that after the recent changes?
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211605825)
These changes are meant to minimize the diffs for the later optimization commits later.
I have added them after you have requested [simplifying a more complicated optimization commit](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1879140048) - these were the parts I was able to extract such that the diff is minimal in the high-risk commits. Later commits can simply change the `vector` to `Obfuscation` because of these refactors. Would you like me to go into more details in the commi
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211605825)
These changes are meant to minimize the diffs for the later optimization commits later.
I have added them after you have requested [simplifying a more complicated optimization commit](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1879140048) - these were the parts I was able to extract such that the diff is minimal in the high-risk commits. Later commits can simply change the `vector` to `Obfuscation` because of these refactors. Would you like me to go into more details in the commi
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211598937)
Let me know if this is still relevant after the previously pushed changes
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211598937)
Let me know if this is still relevant after the previously pushed changes
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211596259)
The two tests are doing different things - one does black-box style property-based testing to validate that certain invariants hold - that deobfuscating an obfuscation results in the original message (higher level, it doesn't have to know about the implementation details).
The second one does what you mentioned, so if I understand you correctly, what you suggest is rather dropping the roundtrip test? I think it's useful because if it fails it gives a higher-level error.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211596259)
The two tests are doing different things - one does black-box style property-based testing to validate that certain invariants hold - that deobfuscating an obfuscation results in the original message (higher level, it doesn't have to know about the implementation details).
The second one does what you mentioned, so if I understand you correctly, what you suggest is rather dropping the roundtrip test? I think it's useful because if it fails it gives a higher-level error.
📝 mzumsande opened a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997)
The logic for DBHashKey / DBHeightKey handling and lookup of entries is shared by coinstatsindex and blockfilterindex, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/key.h` (using templates for the index-specific `DBVal`).
Also remove an unnecessary db query in `CustomAppend()` for the coinstatsindex.
(https://github.com/bitcoin/bitcoin/pull/32997)
The logic for DBHashKey / DBHeightKey handling and lookup of entries is shared by coinstatsindex and blockfilterindex, leading to many lines of duplicated code. De-duplicate this by moving the logic to `index/key.h` (using templates for the index-specific `DBVal`).
Also remove an unnecessary db query in `CustomAppend()` for the coinstatsindex.
💬 achow101 commented on pull request "wallet: Set migrated wallet name only on success":
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2211716770)
Done
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2211716770)
Done
🤔 furszy reviewed a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3027069074)
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3027069074)
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2211768238)
It's not true that they aren't used for anything. The point of reading the block from disk is to check if it's the prev block that is expected because the values that are written for the current block depend on the prev block values. They are used not from the prev block but from the members instead though. I am not sure right now if this check can just be removed, but if they are still needed the expected prev block hash could probably be written into a member too and the DB read could be avoid
...
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2211768238)
It's not true that they aren't used for anything. The point of reading the block from disk is to check if it's the prev block that is expected because the values that are written for the current block depend on the prev block values. They are used not from the prev block but from the members instead though. I am not sure right now if this check can just be removed, but if they are still needed the expected prev block hash could probably be written into a member too and the DB read could be avoid
...
📝 ajtowns opened a pull request: "Bump SCRIPT_VERIFY flags to 64 bit"
(https://github.com/bitcoin/bitcoin/pull/32998)
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](https://github.com/benthecarman/bitcoin/blob/d4a86277ed8a0712e03fbbce290e9209165e049c/src/script/interpreter.h#L175-L195). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type
...
(https://github.com/bitcoin/bitcoin/pull/32998)
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](https://github.com/benthecarman/bitcoin/blob/d4a86277ed8a0712e03fbbce290e9209165e049c/src/script/interpreter.h#L175-L195). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type
...
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3081648267)
The "introduce script_verify_flags typename" commit is probably best reviewed with `--word-diff` fwiw.
cc @instagibbs @darosior @benthecarman
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3081648267)
The "introduce script_verify_flags typename" commit is probably best reviewed with `--word-diff` fwiw.
cc @instagibbs @darosior @benthecarman
💬 nervana21 commented on pull request "log: unify `UpdateTip` values":
(https://github.com/bitcoin/bitcoin/pull/32996#issuecomment-3081669227)
tACK [b039305](https://github.com/bitcoin/bitcoin/pull/32996/commits/b039305d3590aaf84bfdf3b3547b9311897f52c7)
Before:
> 2025-07-16T22:31:33Z UpdateTip: new best=000000000000000000022dabca47b7aabc24d93d0241536ca75dc528d08d5eba height=905352 version=0x20c26000 log2_work=95.713336 tx=1212933015 date='2025-07-13T11:06:38Z' progress=0.998857 cache=8.8MiB(61209txo)
After:
> 2025-07-16T22:54:20Z UpdateTip: new best=0000000000000000000186eed935e4ad209e7aaa4ceeb2542851feaf4b8d426f height=905
...
(https://github.com/bitcoin/bitcoin/pull/32996#issuecomment-3081669227)
tACK [b039305](https://github.com/bitcoin/bitcoin/pull/32996/commits/b039305d3590aaf84bfdf3b3547b9311897f52c7)
Before:
> 2025-07-16T22:31:33Z UpdateTip: new best=000000000000000000022dabca47b7aabc24d93d0241536ca75dc528d08d5eba height=905352 version=0x20c26000 log2_work=95.713336 tx=1212933015 date='2025-07-13T11:06:38Z' progress=0.998857 cache=8.8MiB(61209txo)
After:
> 2025-07-16T22:54:20Z UpdateTip: new best=0000000000000000000186eed935e4ad209e7aaa4ceeb2542851feaf4b8d426f height=905
...