💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283037011)
c7720844a4357aa497362fd5b481bc1a9c27687d: I assume this separation of `CompleteInternal()` is because `EXCLUSIVE_LOCKS_REQUIRED` is only set for `V1Transport` rather than on `Transport`?

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283202700)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463: `NoPendingSend()` ? That makes it more clear this method doesn't mark something done. Also it applies before the first message too.

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283297036)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463: Since we're notifying two different things about these sent bytes - and also setting `nSendSize` - I suggested some extra comments...

```cpp
// Update statistics per message type
```

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283084032)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463 this `LOCK` should have been moved in the previous commit. Otherwise it gives a thread safety warning.

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283265993)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463: this new `assert` and the one below had me a bit worried, but the fuzzer helps and:
1. The very first message it's trivially clear this won't be a problem: `m_sending_header` starts out `false`, `m_bytes_sent` at `0` and `m_message_to_send.data` is initialised empty.
2. `MarkBytesSent` sets these things back when (exactly) all bytes have been put in the send buffer (`vSendMsg`). In v1 this is always the case after the second (header only) or third call
...

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283288152)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463: could add a comment here:

```cpp
// In v1 transport GetBytesToSend first returns a header and next the data (if any).
```

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283305145)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463: could we just assume bytes will get sent this if `GetBytesToSend()` has been called? Afaik there's no way to handle a failure anyway. But I guess it's safer to track it explicitly.

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283298309)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463

```cpp
// Notify Transport that bytes have been processed
```

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283301879)
f97adbf3e93e89b1e8ce1dc212e84ac6b2879463

```cpp
// Update bytes in send buffer
```

(becomes "Update memory use of send buffer" in the next commit)

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1664347932)
> Not entirely sure how to handle that since we don't care about fuzzing BDB itself.

Indeed, @MarcoFalke any idea on how to make the fuzzer ignore that?

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1283499741)
In 8a0cb8b2147e852ac80d2030057272bdb59a83f2: Instead of using `ConsumeDeserializable`, couldn't we have a function to create a `CBlockFileInfo`? E.g.:

```diff
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index b5b25fcbc7..528c2fae9f 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -37,6 +37,19 @@ void init_block_index()
SelectParams(ChainType::MAIN);
}

+CBlockFileInfo CreateCBlockFileInfo(FuzzedDataProvider& fuzze
...

💬 jonatack commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#issuecomment-1664388165)
ACK f054bd072afb72d8dae7adc521ce15c13b236700

👍 theuni approved a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1561610579)
ACK 1c976c691cc4b20f43071aabf36c7afed1571057

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-1664431299)
Thank you for pointing this up, @dergoegge. Actually, I thought it was just a one-line change and became overly relaxed about it. But I'm sorry, and from now on I'll compile before pushing every time.

I also address your style nits in the recent push.

💬 0xB10C commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1664441492)
Concept ACK. I opened issue https://github.com/bitcoin/bitcoin/issues/16721 a while ago but closed it as it didn't get much attention back then.

> I am not aware of anyone reading the `mempool.dat`, is there?

I've written a mempool.dat parser a few years ago for fun. However, as you said, the RPCs are powerful enough and if someone really really wants to read the file, they can implement XOR functionality. Similar to block0000.dat files, these files are not something considered an interf
...

💬 jonatack commented on pull request "refactor: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1283579011)
done

💬 jonatack commented on pull request "refactor: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1283579212)
done

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283582108)
updated the commit message and incorporated brace initialization in latest push

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283584387)
updated in latest push. no longer any references to the bundling in the commit messages

💬 jonatack commented on pull request "refactor: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#issuecomment-1664459802)
Updated the pull description and repushed to take @MarcoFalke's review feedback (thanks!)