refactor,test: follow-ups to multi-byte block obfuscation

l0rinc commented at 5:30 pm on July 22, 2025: contributor

Follow up for #31144 Applied the remaining comments in separate commits - except for the last one where I could group them. Please see the commit messages for more context.

test: merge xor_roundtrip_random_chunks and xor_bytes_reference

Instead of a separate roundtrip test and a simplified xor reference test, we can merge the two and provide the same coverage

See: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211205949

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

a17d8202c3

test: make `obfuscation_serialize` more thorough

See: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216849672

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

2dea045425

refactor: simplify `Obfuscation::HexKey`

See: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2215746554

Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>

298bf95105

refactor: rename `OBFUSCATION_KEY_KEY`

See: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216425882

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

e5b1b7c557

DrahtBot commented at 5:30 pm on July 22, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33039.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, hodlinator, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

l0rinc force-pushed on Jul 22, 2025

l0rinc marked this as ready for review on Jul 22, 2025

in src/dbwrapper.cpp:254 in da7bd4420e outdated

250@@ -251,9 +251,10 @@ CDBWrapper::CDBWrapper(const DBParams& params)
251 
252     assert(!m_obfuscation); // Needed for unobfuscated Read()/Write() below
253     if (!Read(OBFUSCATION_KEY, m_obfuscation) && params.obfuscate && IsEmpty()) {
254-        // Generate, write and read back the new obfuscation key, making sure we don't obfuscate the key itself
255-        Write(OBFUSCATION_KEY, FastRandomContext{}.randbytes(Obfuscation::KEY_SIZE));
256-        Read(OBFUSCATION_KEY, m_obfuscation);
257+        // Generate and write the new obfuscation key - making sure we don't obfuscate the key itself

ryanofsky commented at 7:46 pm on July 23, 2025:

In commit “refactor: standardize obfuscation memory alignment” (e7b00d69b6e05b6b4fd08ace785727c56626cbc2)

Looks good to declare an obfuscation object and let it serialize itself, and not have an unnecessary read call. I still think the “making sure we don’t obfuscate the key” comment is vague, because it’s not clear what it refers to. I’d suggest moving that comment next to the assert and moving the assert where it’s more relevant:

 0--- a/src/dbwrapper.cpp
 1+++ b/src/dbwrapper.cpp
 2@@ -249,10 +249,10 @@ CDBWrapper::CDBWrapper(const DBParams& params)
 3         LogPrintf("Finished database compaction of %s\n", fs::PathToString(params.path));
 4     }
 5 
 6-    assert(!m_obfuscation); // Needed for unobfuscated Read()/Write() below
 7     if (!Read(OBFUSCATION_KEY, m_obfuscation) && params.obfuscate && IsEmpty()) {
 8-        // Generate and write the new obfuscation key - making sure we don't obfuscate the key itself
 9+        // Generate and write the new obfuscation key.
10         const Obfuscation obfuscation{FastRandomContext{}.randbytes<Obfuscation::KEY_SIZE>()};
11+        assert(!m_obfuscation); // Make sure the key is written without obfuscation.
12         Write(OBFUSCATION_KEY, obfuscation);
13         m_obfuscation = obfuscation;
14         LogInfo("Wrote new obfuscation key for %s: %s", fs::PathToString(params.path), m_obfuscation.HexKey());

This also strengthens the assert if the Read method fails in a way leaving m_obfuscation non-zero.

l0rinc commented at 7:57 pm on July 23, 2025:

Done, thanks

ryanofsky approved

ryanofsky commented at 7:50 pm on July 23, 2025: contributor

Code review ACK e7b00d69b6e05b6b4fd08ace785727c56626cbc2. Thanks for the followups!

refactor: write `Obfuscation` object when new key is generated in dbwrapper

See:
* https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2215720251
* https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223539466

Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

13f00345c0

refactor: standardize obfuscation memory alignment

See:
* https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216962117
* https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2220277161
* https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210851772

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

86e3a0a8cb

l0rinc force-pushed on Jul 23, 2025

in src/test/streams_tests.cpp:68 in 2dea045425 outdated

76+
77+    // Test loading a key.
78+    std::vector key_in{m_rng.randbytes<std::byte>(Obfuscation::KEY_SIZE)};
79+    DataStream ds_in;
80+    ds_in << key_in;
81+    BOOST_CHECK_EQUAL(ds_in.size(), 1 + Obfuscation::KEY_SIZE); // serialized as a vector

ryanofsky commented at 8:25 pm on July 23, 2025:

In commit “test: make obfuscation_serialize more thorough” (2dea0454254180d79464dc6afd3312b1caf369a7)

Could check this more strictly with:

 0--- a/src/test/streams_tests.cpp
 1+++ b/src/test/streams_tests.cpp
 2@@ -65,17 +65,19 @@ BOOST_AUTO_TEST_CASE(obfuscation_serialize)
 3     std::vector key_in{m_rng.randbytes<std::byte>(Obfuscation::KEY_SIZE)};
 4     DataStream ds_in;
 5     ds_in << key_in;
 6-    BOOST_CHECK_EQUAL(ds_in.size(), 1 + Obfuscation::KEY_SIZE); // serialized as a vector
 7+    const DataStream key_bytes{ds_in};
 8     ds_in >> obfuscation;
 9 
10     // Test saving the key.
11     std::vector<std::byte> key_out;
12     DataStream ds_out;
13     ds_out << obfuscation;
14+    const DataStream obfuscation_bytes{ds_out};
15     ds_out >> key_out;
16 
17-    // Make sure saved key is the same.
18+    // Make sure key is unchanged, and that key and Obfuscation representations are the same.
19     BOOST_CHECK_EQUAL_COLLECTIONS(key_in.begin(), key_in.end(), key_out.begin(), key_out.end());
20+    BOOST_CHECK_EQUAL_COLLECTIONS(key_bytes.begin(), key_bytes.end(), obfuscation_bytes.begin(), obfuscation_bytes.end());
21 }
22 
23 BOOST_AUTO_TEST_CASE(obfuscation_empty)

l0rinc commented at 8:40 pm on July 23, 2025:

Thanks, I’ll consider it next time I’m pushing

ryanofsky approved

ryanofsky commented at 8:28 pm on July 23, 2025: contributor

Code review ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0, just tweaking key write assert as suggested

in src/util/obfuscation.h:47 in 86e3a0a8cb

46 
47                 target = {std::assume_aligned<KEY_SIZE>(target.data() + alignment), target.size() - alignment};
48                 rot_key = m_rotations[(key_offset + alignment) % KEY_SIZE];
49             }
50-            // Aligned obfuscation in 64-byte chunks
51+            // Aligned obfuscation in 8*KEY_SIZE chunks

hodlinator commented at 1:29 pm on August 2, 2025:

nit:

0            // Aligned obfuscation in 8*KEY_SIZE chunks to nudge compilers toward deploying SIMD

in src/util/obfuscation.h:53 in 86e3a0a8cb

53                 for (size_t i{0}; i < unroll; ++i) {
54                     XorWord(target.subspan(i * KEY_SIZE, KEY_SIZE), rot_key);
55                 }
56             }
57-            // Aligned obfuscation in 64-bit chunks
58+            // Aligned obfuscation in KEY_SIZE chunks

hodlinator commented at 1:30 pm on August 2, 2025:

nit:

0            // Aligned obfuscation of remaining KEY_SIZE chunks

in src/dbwrapper.cpp:1 in e5b1b7c557 outdated

hodlinator commented at 2:32 pm on August 2, 2025:

nit: Commit message for e5b1b7c5577ee36b5bcfb6c02b92da88455411e9 could include a short motivation instead of just the link to the other PR’s comment thread:

0+ Makes more sense as we are no longer serializing the obfuscation data into an intermediate "key"-variable.

hodlinator approved

hodlinator commented at 2:39 pm on August 2, 2025: contributor

ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0

Was worried that removing the random offset parts of the xor_bytes_reference test would remove coverage of the misaligned obfuscation. Instead of running code coverage analysis I applied:

 0diff --git a/src/util/obfuscation.h b/src/util/obfuscation.h
 1index e9a2e6093b..8863af1803 100644
 2--- a/src/util/obfuscation.h
 3+++ b/src/util/obfuscation.h
 4@@ -39,7 +39,7 @@ public:
 5             // Obfuscate until KEY_SIZE alignment boundary
 6             if (const auto misalign{reinterpret_cast<uintptr_t>(target.data()) % KEY_SIZE}) {
 7                 const size_t alignment{KEY_SIZE - misalign};
 8-                XorWord(target.first(alignment), rot_key);
 9+                XorWord(target.first(alignment), 0);
10 
11                 target = {std::assume_aligned<KEY_SIZE>(target.data() + alignment), target.size() - alignment};
12                 rot_key = m_rotations[(key_offset + alignment) % KEY_SIZE];

…and xor_random_chunks catches it due to the random size of chunks being obfuscated.

Reacted to the assert() being moved into the if block in the CDBWrapper-ctor, but it makes sense to rely on the default-initialized state of the member but still validate that Read() hasn’t modified it.

achow101 commented at 10:01 pm on August 6, 2025: member

ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0

achow101 merged this on Aug 6, 2025

achow101 closed this on Aug 6, 2025

l0rinc deleted the branch on Aug 7, 2025

refactor,test: follow-ups to multi-byte block obfuscation #33039

Code Coverage & Benchmarks

Reviews