💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057968526)
Oops!…I Did It Again

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057968721)
Ok, sure, changed it to something similar:
```C++
const bool all_zeros{!obfuscation || std::ranges::all_of(std::span(key_bytes).first(write_size), [](auto b) { return b == std::byte{0}; })};
```

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057968867)
Sure, done

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057969172)
`assert(m_obfuscation == 0)` is needed to document (and to make sure) that the obfuscation key is already written with obfuscation turned on, so we have to make sure the obfuscation key is 0 to turn it off for this write only.

https://github.com/bitcoin/bitcoin/blob/master/src/test/dbwrapper_tests.cpp#L29-L46 demonstrates the usage of this call - I've updated it to verify that reopening the database will read the obfuscation key correctly.

So without `assert(m_obfuscation == 0)` we might n
...

🤔 i-am-yuvi reviewed a pull request: "test: Add missing check for empty stderr in util tester"
(https://github.com/bitcoin/bitcoin/pull/32327#pullrequestreview-2790454279)
tACK fadf12a56c294696052c4cb6ee5284030ada7498

I've reviewed the changes and tested on my system

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2827002303)
> Have you ever encountered IBD failures for code that passes CI? Seems like missing test coverage in that case?

yes, extended one of the tests now, hoping that will cover it next time.

> then chided me in public

Definitely wasn't my intention to scold you in any way, just didn't (and still don't) understand what you're objecting to or suggesting in that part of the code. Pushed some changes, if it's still not clear, let's discuss in person.

💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2827005396)
re-ACK 17bb63f9f9b08e6af60c089234fe878657dbc88e

💬 hebasto commented on issue "gui: translation spam?":
(https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827015633)
I'm currently seeking a volunteer for the position of Polish Translation Coordinator:
- https://x.com/hhebasto/status/1915340627031556112
- https://app.transifex.com/bitcoin/communication/d:f5c2059b-fba0-4546-8b5f-0f7b978bee23/?q=project%3Abitcoin

🤔 pablomartin4btc reviewed a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2790487905)
re-ACK 17bb63f9f9b08e6af60c089234fe878657dbc88e

Since my last review:
- > Added a commit to disable legacy wallet creation from the wallet tool.
- Addresed @davidgumberg's feedback.

👍 hodlinator approved a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-2790161149)
ACK cc8f239663a0874c307efa30815b40d9ef40badb

Fixes thread-unsafe pattern (originally?) raised by me here: https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988127685
(Even though current master is not susceptible to the found `CNode` being deleted randomly on another thread).

💬 hodlinator commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2057808141)
nit: It is somewhat opaque what is going on here. Would be good to add comment:
```suggestion
// Downcast to CNetAddr as we don't care to match on port or other parts of CAddress.
const CNetAddr& net_addr{addr};
```
An even clearer alternative would be to take `CNetAddr` as the function argument and instead clarify this behavior in the comment of the header.

💬 hodlinator commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2057829305)
nit: Should avoid deprecated macro:
```suggestion
LogInfo("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort());
```

💬 hodlinator commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2057856277)
Interesting aspect. Agree current name makes sense - inbound nodes, if they are already connected to us would *not* be using their listening port (even if behind NAT/router). So we could be attempting to establish an outbound connection to a *different* node behind the same IP.

But even if `ConnectNode` here distinguishes between ports, the only caller may override and filter out connections without regard for port, as pointed out above (https://github.com/bitcoin/bitcoin/pull/32326#discussio
...

✅ maflcko closed a pull request: "ci: Temporarily disable failing bpf checks"
(https://github.com/bitcoin/bitcoin/pull/32335)

✅ hebasto closed an issue: "ci: failure in interface_usdt_coinselection.py"
(https://github.com/bitcoin/bitcoin/issues/32322)

🚀 hebasto merged a pull request: "test: Suppress upstream `-Wduplicate-decl-specifier` in bpfcc"
(https://github.com/bitcoin/bitcoin/pull/32336)

💬 0xB10C commented on pull request "test: Suppress upstream `-Wduplicate-decl-specifier` in bpfcc":
(https://github.com/bitcoin/bitcoin/pull/32336#issuecomment-2827076150)
Post merge ACK

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2058025067)
Thanks! (If you re-touch: `std::span{key_bytes}`)

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2058041033)
Ah, I was interpreting "Needed for unobfuscated Read" as applying to `Read`-calls *after* the ctor, hadn't realized it was for the call 2 lines below. :man_facepalming: That's why I didn't think it was relevant to include in https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2057656754. Sorry for this detour. `assert` is good.

Maybe comment could spell out "Needed for unobfuscated Read *directly below*" for similar readers to me, but hopefully there aren't too many of them.

📝 laanwj opened a pull request: "qt: Replace stray tfm::format to cerr with qWarning"
(https://github.com/bitcoin-core/gui/pull/868)
GUI warnings should go to the log, not to the console (which may not be connected at all).