💬 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.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217034784)
Sure, if I push again, I'll consider these nits

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217030868)
If you insist, I'll do it if I have to push again.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217033100)
Can you please point me to a documentation or code which demonstrates that?

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217040108)
Sure, I'll merge those commits next time I push

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3091278861)
Had to rebase after https://github.com/bitcoin/bitcoin/commit/067365d2a8a421a074bb54394118beccb3f775c2#diff-e6100361fa0e9e25478f808ca084e5f681d4dddbbee7b3bea0f9d5bcd29db3aaR39-R563, `src/txorphanage.cpp` was moved to `src/node/txorphanage.cpp`, the change is exactly the same otherwise, would appreciate re-reviews.

💬 achow101 commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3091292177)
ACK 248b6a27c351690d3596711cc36b8102977adeab