💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216425882)
In commit "scripted-diff: unify xor-vs-obfuscation nomenclature" (0b8bec8aa6260c499c2663ab7a1c905da0d312c3)

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

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216849672)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211613705

> 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:

```c++
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)};
DataStr
...

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216948127)
In commit "optimization: peel align-head and unroll body to 64 bytes" (248b6a27c351690d3596711cc36b8102977adeab)

reinterpret_cast seems more appropriate than bit_cast here, though either are probably ok. IIUC reinterpret cast is guaranteed to convert the pointer to an integer that can be round tripped back into a valid pointer, while bit_cast is looser and doesn't provide any guarantees.

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216935443)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210851772

In commit "optimization: peel align-head and unroll body to 64 bytes" (248b6a27c351690d3596711cc36b8102977adeab)

I was about to ask the same question about why min was there to handle an impossible condition and would suggest either removing the `min` or removing the `if (target.size() > KEY_SIZE)` check if the latter wouldn't hurt performance . Either of these would make the code easier to follow, IMO

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216962117)
In commit "optimization: peel align-head and unroll body to 64 bytes" (248b6a27c351690d3596711cc36b8102977adeab)

Might be clearer to s/64-byte/8*KEY_SIZE/ here and s/64-bit/KEY_SIZE/ below

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216850853)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211627396

Thanks! New commit and explanations make this much clearer to me

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216460569)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211605825

In commit "refactor: prepare `DBWrapper` for obfuscation key change" (6bbf2d9311b47a8a15c17d9fe11828ee623d98e0)

> I have added them after you have requested [simplifying a more complicated optimization commit](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1879140048)

Thanks for trying this. Looking at these two "prepare for" commits I don't think they simply the optimization commit and just create compl
...

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216848225)
re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211610091

> Do you recommend we apply any of that after the recent changes?

Nope, new implementation looks better and this should be resolved.

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2216198711)
In commit "test: compare util::Xor with randomized inputs against simple impl" (618a30e326e9bcfd72e0e2645ce49f8b2a88714d)

re: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211596259

Thanks for the detail in the commit message clarifying the reasons for the changes.

I would still suggest simplifying the tests. I think the first test is simple and easy to understand but has a major gap in coverage, and the second test provides good coverage but is written in a convoluted styl
...

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3090820186)
Fixed conflicts and rebased to master.

🤔 fjahr reviewed a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3034913148)
Looks good, aside from the documentation cleanup in `blockfilterindex.cpp` you can consider my comments optional.

💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216989959)
The comment above here still talks about three items stored for each block but after the change only one of the values is here. This would need to be adopted somehow and probably some of the docs moved over to `index/key.h`.

EDIT: Maybe the current comment can be left almost untouched but there needs to be a reference to where the keys are now.

💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216982231)
nit: could do IWYU I guess, even though this wasn't followed that well in the index cpp files.

```
#include <dbwrapper.h>
#include <interfaces/types.h>
```

💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216992529)
Also, very minor naming nit: File could be called `db_key.h` since we have other kinds of keys but I don't feel too strongly about it.

💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216991906)
Aside from the documentation moving necessary mentioned above maybe there could be 1-2 general sentences here that this deals with the hash/height keys and that it's mostly relevant to indexes that use these.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217023785)
Ok, will take @maflcko's suggestion if I have to push again.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217028864)
Did that before, but @hodlinator pointed out that the previous version made more sense since it's storing obfuscation key's database key: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2068663871.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217024259)
> The simplest way to write this would be:
> m_obfuscation = Obfuscation{FastRandomContext{}.randbytes<Obfuscation::KEY_SIZE>()};
> Write(OBFUSCATION_KEY_KEY, m_obfuscation);

I don't think so, `Write` needs `m_obfuscation`, so it would obfuscate the key itself if we assign it before the write, try `build/bin/test_bitcoin --run_test=dbwrapper_tests`.
Seems the comment I added still isn't helping, how do I rephrase it to make this part obvious?

```C++
const Obfuscation new_key{FastRandom
...

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217039648)
Not sure I understand why this is better, we have the xor instructions spread to multiple places now - apply_random_xor_chunks & inline `original ^ key_bytes`. What do others think?

🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3034969188)
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.