💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529426)
Let me know what you think of the result now.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529426)
Let me know what you think of the result now.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529189)
> m_obfuscation = Obfuscation{FastRandomContext{}.randbytes<8>()};
Write(OBFUSCATION_KEY_KEY, m_obfuscation);
If we enable obfuscation (by assigning the field) before calling `Write`, it will obfuscate the new key *with* the new key in `CDBBatch::WriteImpl`, so before writing we need `m_obfuscation` to remain empty. I've extended the comments to clarify that.
> FastRandomContext calls GetRandBytes internally
Nice, if we can indeed use `FastRandomContext{}`, the code can be simplified a
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529189)
> m_obfuscation = Obfuscation{FastRandomContext{}.randbytes<8>()};
Write(OBFUSCATION_KEY_KEY, m_obfuscation);
If we enable obfuscation (by assigning the field) before calling `Write`, it will obfuscate the new key *with* the new key in `CDBBatch::WriteImpl`, so before writing we need `m_obfuscation` to remain empty. I've extended the comments to clarify that.
> FastRandomContext calls GetRandBytes internally
Nice, if we can indeed use `FastRandomContext{}`, the code can be simplified a
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529040)
Removed from intermediary commit
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529040)
Removed from intermediary commit
🤔 ryanofsky reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026270097)
Code review 7b93fa394acaee7a715fae9a2b968d33c5adc174. Still in progress reviewing but wanted to post comments I had so far. There was a new push since these were written so please ignore anything that no longer applies.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026270097)
Code review 7b93fa394acaee7a715fae9a2b968d33c5adc174. Still in progress reviewing but wanted to post comments I had so far. There was a new push since these were written so please ignore anything that no longer applies.
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211205949)
In commit "test: compare util::Xor with randomized inputs against simple impl" (7750bfe17e1c5cb69806f9f83675cd1e78083f7f)
This seems like a weak check which is not actually verifying the obfuscation is done correctly. Why not just check that obfuscated data is correct?
```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -37,9 +37,10 @@ BOOST_AUTO_TEST_CASE(xor_roundtrip_random_chunks)
const auto key_bytes{m_rng.randbool() ? m_rng.randbytes<Obfuscation:
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211205949)
In commit "test: compare util::Xor with randomized inputs against simple impl" (7750bfe17e1c5cb69806f9f83675cd1e78083f7f)
This seems like a weak check which is not actually verifying the obfuscation is done correctly. Why not just check that obfuscated data is correct?
```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -37,9 +37,10 @@ BOOST_AUTO_TEST_CASE(xor_roundtrip_random_chunks)
const auto key_bytes{m_rng.randbool() ? m_rng.randbytes<Obfuscation:
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211268495)
In commit "test: compare util::Xor with randomized inputs against simple impl" (7750bfe17e1c5cb69806f9f83675cd1e78083f7f)
There are a lot of changes in this commit which seem to be made without a clear reason and aren't mentioned at all in the commit message.
I understand the point of adding the new `xor_roundtrip_random_chunks` and `xor_bytes_reference` tests but none of the other changes including the rewrite of the main `dbwrapper` test at the top, and the random tweaks such as this see
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211268495)
In commit "test: compare util::Xor with randomized inputs against simple impl" (7750bfe17e1c5cb69806f9f83675cd1e78083f7f)
There are a lot of changes in this commit which seem to be made without a clear reason and aren't mentioned at all in the commit message.
I understand the point of adding the new `xor_roundtrip_random_chunks` and `xor_bytes_reference` tests but none of the other changes including the rewrite of the main `dbwrapper` test at the top, and the random tweaks such as this see
...
💬 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.