💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283479802)
Thanks for pointing this out 🙏 I agree it shouldn't be possible for this test to succeed so an exception should be raised.
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283479802)
Thanks for pointing this out 🙏 I agree it shouldn't be possible for this test to succeed so an exception should be raised.
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1664326798)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1664326798)
Concept ACK
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1664327227)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1664327227)
Concept ACK
🚀 fanquake merged a pull request: "ci: Move ASan USDT to persistent_worker"
(https://github.com/bitcoin/bitcoin/pull/28161)
(https://github.com/bitcoin/bitcoin/pull/28161)
💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1664336561)
What's the root cause that makes a transaction need Replace-by-fee in the first place? Can we fix that?
RBF is a feature for consenting adults. If you don’t want to participate in it, you don’t need to.
Your passion for it isn’t a reason to force others into using it in transactions by default that don’t involve you.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1664336561)
What's the root cause that makes a transaction need Replace-by-fee in the first place? Can we fix that?
RBF is a feature for consenting adults. If you don’t want to participate in it, you don’t need to.
Your passion for it isn’t a reason to force others into using it in transactions by default that don’t involve you.
💬 Sjors commented on pull request "BIP324 connection support":
(https://github.com/bitcoin/bitcoin/pull/28196#issuecomment-1664337914)
Concept ACK
Mostly happy with b7a7ed70d06ab3f994ff58e3a0c99105ee88ab6e. Minor inline suggestions and/or making sure I understand what the code is doing.
I also checked that all intermediate commits compile and ran the new fuzzer for a few hours (but didn't study it very carefully).
* 1937a5fcf795149c44b7f4f016c05000ac3adaf9: `-maxsendbuffer` help could be changed to say "Maximum per-connection memory use for the send buffer"
* 68e48a0185751d24eecb194b8efd7028c8b590f3: can you elaborate
...
(https://github.com/bitcoin/bitcoin/pull/28196#issuecomment-1664337914)
Concept ACK
Mostly happy with b7a7ed70d06ab3f994ff58e3a0c99105ee88ab6e. Minor inline suggestions and/or making sure I understand what the code is doing.
I also checked that all intermediate commits compile and ran the new fuzzer for a few hours (but didn't study it very carefully).
* 1937a5fcf795149c44b7f4f016c05000ac3adaf9: `-maxsendbuffer` help could be changed to say "Maximum per-connection memory use for the send buffer"
* 68e48a0185751d24eecb194b8efd7028c8b590f3: can you elaborate
...
🤔 Sjors reviewed a pull request: "net: transport abstraction"
(https://github.com/bitcoin/bitcoin/pull/28165#pullrequestreview-1560842698)
Concept ACK
Mostly happy with b7a7ed70d06ab3f994ff58e3a0c99105ee88ab6e. Minor inline suggestions and/or making sure I understand what the code is doing.
I also checked that all intermediate commits compile and ran the new fuzzer for a few hours (but didn't study it very carefully).
* 1937a5fcf795149c44b7f4f016c05000ac3adaf9: `-maxsendbuffer` help could be changed to say "Maximum per-connection memory use for the send buffer"
* 68e48a0185751d24eecb194b8efd7028c8b590f3: can you elaborate
...
(https://github.com/bitcoin/bitcoin/pull/28165#pullrequestreview-1560842698)
Concept ACK
Mostly happy with b7a7ed70d06ab3f994ff58e3a0c99105ee88ab6e. Minor inline suggestions and/or making sure I understand what the code is doing.
I also checked that all intermediate commits compile and ran the new fuzzer for a few hours (but didn't study it very carefully).
* 1937a5fcf795149c44b7f4f016c05000ac3adaf9: `-maxsendbuffer` help could be changed to say "Maximum per-connection memory use for the send buffer"
* 68e48a0185751d24eecb194b8efd7028c8b590f3: can you elaborate
...
💬 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`?
(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.
(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
```
(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.
(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
...
(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).
```
(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.
(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
```
(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)
(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?
(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
...
(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
(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
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1561610579)
ACK 1c976c691cc4b20f43071aabf36c7afed1571057