[IBD] multi-byte block obfuscation#31144
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31144. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
b76314a to
10b9e68
Compare
|
I think your example may be a bit skewed? It shows how much time is spent when deserializing a If you want the micro-benchmark to be representative, I'd presume you'd have to mimic the histogram of the sizes of writes. Just picking one (1024, or 1004), which is never hit in reality, and then optimizing for that may be misleading. |
23f4809 to
6ae466b
Compare
Thanks, I've extended your previous benchmarks with both Autofile serialization and very small vectors. |
353915b to
a3dc138
Compare
|
It would be good to explain the jpegs in the description, or even remove them. They will be excluded from the merge commit and aren't shown, unless GitHub happens to be reachable and online. Are they saying that IBD was 4% faster? Also, I think they were created with the UB version of this pull, so may be outdated either way? I did a quick check on my laptop and it seems the I can try to bench on another machine later, to see if it makes a difference. Can you clarify what type of machine you tested this on? |
That's what I'm measuring currently, but I don't expect more than 2% difference here.
Benchmarks indicated that the 64 bit compiled result was basically the same.
I'll investigate, thanks. DetailsPosting the perf here for reference: |
hodlinator
left a comment
There was a problem hiding this comment.
Concept ACK a3dc138798e2f2c7aa1c9b37633c16c1b523a251
Operating on CPU words rather than individual bytes. 👍
Not entirely clear to me from #31144 (comment) whether the optimizer is able to use SIMD. Guess picking through the binary of a GUIX-build would give a definitive answer.
The verbosity of std::memcpy hurts readability but alignment issues are real.
95e38ac to
cd61e8b
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
cd61e8b to
d72968f
Compare
std::vector<std::byte>(8) to uint64_t
d72968f to
555f8c8
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
maflcko
left a comment
There was a problem hiding this comment.
lgtm ack 7b93fa394acaee7a715fae9a2b968d33c5adc174 📕
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ack 7b93fa394acaee7a715fae9a2b968d33c5adc174 📕
oZkgF1tzWxCxE4D9fRIXf9lAc9P9X7sxHugu5QW4rogkU1EhQ/XW6zHnGVGtAXhrw82ClIvoDbDWH5ksSwyiDg==
There was a problem hiding this comment.
nit in 43189b8: Is exposing this implementation detail really needed? This is adding more lines to real code than there are test-only lines that need it. There are two tests:
- One that checks the
ToKeyround-trip. However, this is already checked by the existing tests and the serialize test can instead do the comparison on aDataStream, if needed: Check that a std::vector (de)serialization (roundtrip) is equal to an obfuscation (de)serialization (roundtrip). - One that checks a serialize round-trip. However, this would be covered by the above suggested test as well.
There was a problem hiding this comment.
Yes, it's redundant, but I wanted to make absolutely sure the internals are also correct.
If other reviewers also comfortable deleting this, I don't mind.
There was a problem hiding this comment.
It only checks m_rotations[0] against a hardcoded memcpy, it doesn't check ToKey in the top commit, so I don't think the unit test adds any new checks. Otherwise, it would be good to know what exact bug this could possibly catch that the other test isn't catching.
In fact the unit test seems a bit confusing, because it constructs the obfuscation from an uint64_t, whereas production code only uses byte spans.
If you really want to directly test Serialize uses m_rotations[0] and Unserialize or the constructor use ToKey, so I am thinking using those public functions is a better way to test the internals than to introduce new and duplicate test-only function.
edit: Also putting a type named generally "KeyType" in the global namespace for this test-only feature seems a bit confusing. If you really want to test the internal state, my preference would be to mark the private field protected and then in the unit test code expose the protected field in a derived class. (But my first preference would still be the above 😅 )
There was a problem hiding this comment.
Let me know what you think of the result now.
There was a problem hiding this comment.
re: #31144 (comment)
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.
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 KeyType = uint64_t;
- constexpr KeyType test_key{0x0123456789ABCDEF};
-
- std::array<std::byte, Obfuscation::KEY_SIZE> key_bytes{};
- std::memcpy(key_bytes.data(), &test_key, Obfuscation::KEY_SIZE);
- const Obfuscation obfuscation{key_bytes};
- BOOST_CHECK_EQUAL(obfuscation_private::Key(obfuscation), test_key);
-}
-
BOOST_AUTO_TEST_CASE(obfuscation_serialize)
{
- const Obfuscation original{m_rng.randbytes<Obfuscation::KEY_SIZE>()};
+ Obfuscation obfuscation{};
- // Serialize
- DataStream ds;
- ds << original;
+ // Test loading a key.
+ std::vector<std::byte> key_in{m_rng.randbytes<std::byte>(Obfuscation::KEY_SIZE)};
+ DataStream ds_in;
+ ds_in << key_in;
+ ds_in >> obfuscation;
- BOOST_CHECK_EQUAL(ds.size(), 1 + Obfuscation::KEY_SIZE); // serialized as a vector
+ // Test saving the key.
+ std::vector<std::byte> key_out;
+ DataStream ds_out;
+ ds_out << obfuscation;
+ ds_out >> key_out;
- // Deserialize
- Obfuscation recovered{};
- ds >> recovered;
-
- BOOST_CHECK_EQUAL(obfuscation_private::Key(recovered), obfuscation_private::Key(original));
+ // Make sure saved key is the same.
+ BOOST_CHECK_EQUAL_COLLECTIONS(key_in.begin(), key_in.end(), key_out.begin(), key_out.end());
}
BOOST_AUTO_TEST_CASE(obfuscation_empty)
--- a/src/util/obfuscation.h
+++ b/src/util/obfuscation.h
@@ -15,17 +15,10 @@
#include <ios>
#include <memory>
-using KeyType = uint64_t;
-
-class Obfuscation;
-
-namespace obfuscation_private {
-KeyType Key(const Obfuscation& obfuscation);
-}
-
class Obfuscation
{
public:
+ using KeyType = uint64_t; //!< Internal key representation.
static constexpr size_t KEY_SIZE{sizeof(KeyType)};
Obfuscation() { SetRotations(0); }
@@ -111,13 +104,7 @@ private:
raw ^= key;
std::memcpy(target.data(), &raw, target.size());
}
-
- friend KeyType obfuscation_private::Key(const Obfuscation& obfuscation);
};
-inline KeyType obfuscation_private::Key(const Obfuscation& obfuscation)
-{
- return obfuscation.m_rotations[0];
-}
#endif // BITCOIN_UTIL_OBFUSCATION_HThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
re: #31144 (comment)
I think the current way is probably simpler. Please let me know if you disagree.
I think current way is ok, but serialization test in the diff has some benefits over the current one. Diff suggested:
BOOST_AUTO_TEST_CASE(obfuscation_serialize)
{
Obfuscation obfuscation{};
// Test loading a key.
std::vector<std::byte> key_in{m_rng.randbytes<std::byte>(Obfuscation::KEY_SIZE)};
DataStream ds_in;
ds_in << key_in;
ds_in >> obfuscation;
// Test saving the key.
std::vector<std::byte> key_out;
DataStream ds_out;
ds_out << obfuscation;
ds_out >> key_out;
// Make sure saved key is the same.
BOOST_CHECK_EQUAL_COLLECTIONS(key_in.begin(), key_in.end(), key_out.begin(), key_out.end());
}IMO this was better because it actually checks output of serialization is as expected, not just that it round-trips and is a certain length. Also think it's better to not use HexStr/HexKey to check for equality. But feel free to keep the current test.
There was a problem hiding this comment.
Added it to the test suite: 2dea045 (#33039)
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Since 31 byte xor-keys are not used in the codebase, using the common size (8 bytes) makes the benchmarks more realistic. Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
There was a problem hiding this comment.
Thanks for the hints, the latest version simplifies reading the obfuscation key by using the new Obfuscation object's serialization paths + reading back the new key directly after generating a new one.
The obfuscation_constructors test was also changes to validate the new HexKey conversion which we need since we're logging the generated obfuscations keys so we need to peek into the object.
It also avoids a possible Obfuscation copy used in GetObfuscation and removed the obfuscation_private::Key friend accessor - which was ugly and we don't need it now that HexKey was exposed.
There was a problem hiding this comment.
Let me know what you think of the result now.
ryanofsky
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
re: #31144 (comment)
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.
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 KeyType = uint64_t;
- constexpr KeyType test_key{0x0123456789ABCDEF};
-
- std::array<std::byte, Obfuscation::KEY_SIZE> key_bytes{};
- std::memcpy(key_bytes.data(), &test_key, Obfuscation::KEY_SIZE);
- const Obfuscation obfuscation{key_bytes};
- BOOST_CHECK_EQUAL(obfuscation_private::Key(obfuscation), test_key);
-}
-
BOOST_AUTO_TEST_CASE(obfuscation_serialize)
{
- const Obfuscation original{m_rng.randbytes<Obfuscation::KEY_SIZE>()};
+ Obfuscation obfuscation{};
- // Serialize
- DataStream ds;
- ds << original;
+ // Test loading a key.
+ std::vector<std::byte> key_in{m_rng.randbytes<std::byte>(Obfuscation::KEY_SIZE)};
+ DataStream ds_in;
+ ds_in << key_in;
+ ds_in >> obfuscation;
- BOOST_CHECK_EQUAL(ds.size(), 1 + Obfuscation::KEY_SIZE); // serialized as a vector
+ // Test saving the key.
+ std::vector<std::byte> key_out;
+ DataStream ds_out;
+ ds_out << obfuscation;
+ ds_out >> key_out;
- // Deserialize
- Obfuscation recovered{};
- ds >> recovered;
-
- BOOST_CHECK_EQUAL(obfuscation_private::Key(recovered), obfuscation_private::Key(original));
+ // Make sure saved key is the same.
+ BOOST_CHECK_EQUAL_COLLECTIONS(key_in.begin(), key_in.end(), key_out.begin(), key_out.end());
}
BOOST_AUTO_TEST_CASE(obfuscation_empty)
--- a/src/util/obfuscation.h
+++ b/src/util/obfuscation.h
@@ -15,17 +15,10 @@
#include <ios>
#include <memory>
-using KeyType = uint64_t;
-
-class Obfuscation;
-
-namespace obfuscation_private {
-KeyType Key(const Obfuscation& obfuscation);
-}
-
class Obfuscation
{
public:
+ using KeyType = uint64_t; //!< Internal key representation.
static constexpr size_t KEY_SIZE{sizeof(KeyType)};
Obfuscation() { SetRotations(0); }
@@ -111,13 +104,7 @@ private:
raw ^= key;
std::memcpy(target.data(), &raw, target.size());
}
-
- friend KeyType obfuscation_private::Key(const Obfuscation& obfuscation);
};
-inline KeyType obfuscation_private::Key(const Obfuscation& obfuscation)
-{
- return obfuscation.m_rotations[0];
-}
#endif // BITCOIN_UTIL_OBFUSCATION_HThe two tests are doing different things - `xor_roundtrip_random_chunks` 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 `xor_bytes_reference` test makes sure the optimized xor implementation behaves in every imaginable scenario exactly as the simplest possible obfuscation - with random chunks, random alignment, random data, random key. Since we're touching the file, other related small refactors were also applied: * `nullpt` typo fixed; * manual byte-by-byte xor key creations were replaced with `_hex` factories; * since we're only using 64 bit keys in production, smaller keys were changed to reflect real-world usage; Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
A previous PR already solved the tiny byte-array-xors during serialization, so it makes sense to keep focusing on the performance of bigger continuous chunks. This also renames the file from `xor` to `obfuscation` to enable scripted diff name unification later. > C++ compiler .......................... GNU 14.2.0 | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 0.84 | 1,184,138,235.64 | 0.0% | 9.01 | 3.03 | 2.971 | 1.00 | 0.1% | 5.50 | `ObfuscationBench` > C++ compiler .......................... Clang 20.1.7 | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 0.89 | 1,124,087,330.23 | 0.1% | 6.52 | 3.20 | 2.041 | 0.50 | 0.2% | 5.50 | `ObfuscationBench`
Mechanical refactor of the low-level "xor" wording to signal the intent instead of the implementation used. The renames are ordered by heaviest-hitting substitutions first, and were constructed such that after each replacement the code is still compilable. -BEGIN VERIFY SCRIPT- sed -i \ -e 's/\bGetObfuscateKey\b/GetObfuscation/g' \ -e 's/\bxor_key\b/obfuscation/g' \ -e 's/\bxor_pat\b/obfuscation/g' \ -e 's/\bm_xor_key\b/m_obfuscation/g' \ -e 's/\bm_xor\b/m_obfuscation/g' \ -e 's/\bobfuscate_key\b/m_obfuscation/g' \ -e 's/\bOBFUSCATE_KEY_KEY\b/OBFUSCATION_KEY_KEY/g' \ -e 's/\bSetXor(/SetObfuscation(/g' \ -e 's/\bdata_xor\b/obfuscation/g' \ -e 's/\bCreateObfuscateKey\b/CreateObfuscation/g' \ -e 's/\bobfuscate key\b/obfuscation key/g' \ $(git ls-files '*.cpp' '*.h') -END VERIFY SCRIPT-
Since `FastRandomContext` delegates to `GetRandBytes` anyway, we can simplify new key generation to a Write/Read combo, unifying the flow of enabling obfuscation via `Read`. The comments were also adjusted to clarify that the `m_obfuscation` field affects the behavior of `Read` and `Write` methods. These changes are meant to simplify the diffs for the riskier optimization commits later.
These changes are meant to simplify the diffs for the riskier optimization commits later.
This is meant to focus the usages to narrow the scope of the obfuscation optimization. `Obfuscation::Xor` is mostly a move. Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
…yte>` to `uint64_t` All former `std::vector<std::byte>` keys were replaced with `uint64_t` (we still serialize them as vectors but convert immediately to `uint64_t` on load). This is why some tests still generate vector keys and convert them to `uint64_t` later instead of generating them directly. In `Obfuscation::Unserialize` we can safely throw an `std::ios_base::failure` since during mempool fuzzing `mempool_persist.cpp#L141` catches and ignored these errors. > C++ compiler .......................... GNU 14.2.0 | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 0.04 | 28,365,698,819.44 | 0.0% | 0.34 | 0.13 | 2.714 | 0.07 | 0.0% | 5.33 | `ObfuscationBench` > C++ compiler .......................... Clang 20.1.7 | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 0.08 | 13,012,464,203.00 | 0.0% | 0.65 | 0.28 | 2.338 | 0.13 | 0.8% | 5.50 | `ObfuscationBench` Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Benchmarks indicated that obfuscating multiple bytes already gives an order of magnitude speed-up, but: * GCC still emitted scalar code; * Clang’s auto-vectorized loop ran on the slow unaligned-load path. Fix contains: * peeling the misaligned head enabled the hot loop starting at an 8-byte address; * `std::assume_aligned<8>` tells the optimizer the promise holds - required to keep Apple Clang happy; * manually unrolling the body to 64 bytes enabled GCC to auto-vectorize. Note that `target.size() > KEY_SIZE` condition is just an optimization, the aligned and unaligned loops work without it as well - it's why the alignment calculation still contains `std::min`. > C++ compiler .......................... GNU 14.2.0 | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 0.03 | 32,464,658,919.11 | 0.0% | 0.50 | 0.11 | 4.474 | 0.08 | 0.0% | 5.29 | `ObfuscationBench` > C++ compiler .......................... Clang 20.1.7 | ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 0.02 | 41,231,547,045.17 | 0.0% | 0.30 | 0.09 | 3.463 | 0.02 | 0.0% | 5.47 | `ObfuscationBench` Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
There was a problem hiding this comment.
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.
maflcko
left a comment
There was a problem hiding this comment.
Not sure the code is correct (see comment), but I've looked at it:
review ACK 248b6a2 🎻
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 248b6a27c351690d3596711cc36b8102977adeab 🎻
y+Vf1FMMs+Mh/qNuFt7m5rGXcMVrJvCxgLk26KJE7ErP9Qbcwd7mBpBp7xxVtWzxWIsB66iJvW59yuotufVoBg==
| std::string HexKey() const | ||
| { | ||
| return HexStr(m_key); | ||
| return HexStr(std::bit_cast<std::array<uint8_t, KEY_SIZE>>(m_rotations[0])); |
There was a problem hiding this comment.
nit in e7114fc: Not that it matters, but you can also use a span to avoid the copy:
- return HexStr(std::bit_cast<std::array<uint8_t, KEY_SIZE>>(m_rotations[0]));
+ return HexStr(std::as_bytes(std::span{&m_rotations[0], 1})));There was a problem hiding this comment.
Yeah, I know we're using these in serialization.h, but it seems to me the current option might be easier to optimize, see https://godbolt.org/z/fh8o9xdvz
It's possible my reproducer is not representative, I also don't mind switching if others prefer it.
There was a problem hiding this comment.
re: #31144 (comment)
as_bytes also seems a little clearer to me and might suggest a variation HexStr(std::as_bytes(std::span{m_rotations}.first(1)));, but no strong opinion. I'd be interested to understand godbolt example too, not sure if there is a simple explantion of why bit_cast might help?
There was a problem hiding this comment.
Ok, not sure it matters, but can take @maflcko's suggestion if I have to push again.
There was a problem hiding this comment.
Done in a separate PR, thanks: 298bf95 (#33039)
| m_obfuscation = std::vector<unsigned char>(Obfuscation::KEY_SIZE, '\000'); | ||
|
|
||
| bool key_exists = Read(OBFUSCATE_KEY_KEY, obfuscate_key); | ||
| bool key_exists = Read(OBFUSCATION_KEY_KEY, m_obfuscation); |
There was a problem hiding this comment.
In commit "scripted-diff: unify xor-vs-obfuscation nomenclature" (0b8bec8)
Given latest changes where the obfuscation object is what's serialized instead of the key (e.g. Read(OBFUSCATION_KEY_KEY, m_obfuscation)), I think it probably makes sense to rename OBFUSCATE_KEY_KEY to OBFUSCATION_KEY
There was a problem hiding this comment.
Did that before, but @hodlinator pointed out that the previous version made more sense since it's storing obfuscation key's database key: #31144 (comment).
There was a problem hiding this comment.
re: #31144 (comment)
Did that before, but @hodlinator pointed out that the previous version made more sense since it's storing obfuscation key's database key: #31144 (comment).
Yes, but that was when the row contained was read as a key rather than an Obfuscation object. IMO, if the row contains a serialized Obfuscation object the key should be called OBFUSCATION_KEY, and if contains a serialized obfuscation key OBFUSCATION_KEY_KEY is a reasonable name. Prior to the most recent push the Obfuscation class had serialization methods but they were not used here, so now that they are used I think the OBFUSCATION_KEY name probably makes more sense.
There was a problem hiding this comment.
Sure, done in a separate PR: e5b1b7c (#33039)
| std::string HexKey() const | ||
| { | ||
| return HexStr(m_key); | ||
| return HexStr(std::bit_cast<std::array<uint8_t, KEY_SIZE>>(m_rotations[0])); |
There was a problem hiding this comment.
re: #31144 (comment)
as_bytes also seems a little clearer to me and might suggest a variation HexStr(std::as_bytes(std::span{m_rotations}.first(1)));, but no strong opinion. I'd be interested to understand godbolt example too, not sure if there is a simple explantion of why bit_cast might help?
l0rinc
left a comment
There was a problem hiding this comment.
Thanks for the review.
Since the PR is in review for almost a year now, and since the remaining comments seem like they can potentially be done in follow-ups as well, I'd prefer getting this over the finish line soon.
| std::string HexKey() const | ||
| { | ||
| return HexStr(m_key); | ||
| return HexStr(std::bit_cast<std::array<uint8_t, KEY_SIZE>>(m_rotations[0])); |
There was a problem hiding this comment.
Ok, not sure it matters, but can take @maflcko's suggestion if I have to push again.
| m_obfuscation = std::vector<unsigned char>(Obfuscation::KEY_SIZE, '\000'); | ||
|
|
||
| bool key_exists = Read(OBFUSCATE_KEY_KEY, obfuscate_key); | ||
| bool key_exists = Read(OBFUSCATION_KEY_KEY, m_obfuscation); |
There was a problem hiding this comment.
Did that before, but @hodlinator pointed out that the previous version made more sense since it's storing obfuscation key's database key: #31144 (comment).
|
ACK 248b6a2 |
|
Thanks a lot @maflcko, @ryanofsky, @hodlinator and @achow101 for all the reviews and reproducers and suggestions! |
| std::string HexKey() const | ||
| { | ||
| return HexStr(m_key); | ||
| return HexStr(std::bit_cast<std::array<uint8_t, KEY_SIZE>>(m_rotations[0])); |
There was a problem hiding this comment.
Done in a separate PR, thanks: 298bf95 (#33039)
| m_obfuscation = std::vector<unsigned char>(Obfuscation::KEY_SIZE, '\000'); | ||
|
|
||
| bool key_exists = Read(OBFUSCATE_KEY_KEY, obfuscate_key); | ||
| bool key_exists = Read(OBFUSCATION_KEY_KEY, m_obfuscation); |
There was a problem hiding this comment.
Sure, done in a separate PR: e5b1b7c (#33039)
There was a problem hiding this comment.
Added it to the test suite: 2dea045 (#33039)
This change is part of [IBD] - Tracking PR for speeding up Initial Block Download
Summary
Current block obfuscations are done byte-by-byte, this PR batches them to 64 bit primitives to speed up obfuscating bigger memory batches.
This is especially relevant now that #31551 was merged, having bigger obfuscatable chunks.
Since this obfuscation is optional, the speedup measured here depends on whether it's a random value or completely turned off (i.e. XOR-ing with 0).
Changes in testing, benchmarking and implementation
std::vector<std::byte>(8)keys to plainuint64_t;Assembly
Memory alignment is enforced by a small peel-loop (
std::memcpyis optimized out on tested platform), with anstd::assume_aligned<8>check, see the Godbolt listing at https://godbolt.org/z/59EMv7h6Y for detailsDetails
Endianness
The only endianness issue was with bit rotation, intended to realign the key if obfuscation halted before full key consumption.
Elsewhere, memory is read, processed, and written back in the same endianness, preserving byte order.
Since CI lacks a big-endian machine, testing was done locally via Docker.
Details
Measurements (micro benchmarks and full IBDs)
GNU 14.2.0
ObfuscationBenchObfuscationBenchObfuscationBenchClang 20.1.7
ObfuscationBenchObfuscationBenchObfuscationBenchi.e. 27.4x faster obfuscation with GCC, 36.7x faster with Clang
For other benchmark speedups see https://corecheck.dev/bitcoin/bitcoin/pulls/31144
Running an IBD until 888888 blocks reveals a 4% speedup.
Details
SSD: