💬 paplorinc commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283470743)
won't this pass if no exception is raised?
Shouldn't we add something like either:
```Python
try:
self.do_test_k_of_n(1, 1000)
assert False, "Expected exception was not raised for 1-of-1000 test"
except JSONRPCException as e:
```

or via unittest:
```Python
# Test 1-of-1000, should not succeed
with self.assertRaises(JSONRPCException) as cm:
self.do_test_k_of_n(1, 1000)
self.assertEqual(cm.exception.error["code"], -5)
self.assertIn("Cannot have 1000 keys in multi_a; mus
...

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664316581)
> From: https://cirrus-ci.com/task/6097246463197184
> ...

Nice, thanks!

💬 fanquake commented on pull request "build: Bump minimum supported Clang to clang-13":
(https://github.com/bitcoin/bitcoin/pull/28210#issuecomment-1664317915)
> This fails on macOS 11.0,

Yea, this'll at least need to wait for the macOS compiler bump, but the next time that happens (soon) it will be from 11 to 16, if not 17.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664319251)
Dropped the test commit back out.

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

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(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

🚀 fanquake merged a pull request: "ci: Move ASan USDT to persistent_worker"
(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.

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

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

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