💬 instagibbs commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283672181)
if something should raise an error and a specific code/message, you use `assert_raises_rpc_error`. Just take a look on how it's used elsewhere?

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1283674053)
Actually, that name would be confusing for V2, where it's possible that there is no pending message, but also no message can be provided (yet) because the handshake has not completed.

Perhaps `CanSetNewMessageToSend()` is better? (let the bikesheddening begin!)

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1283682048)
The `ForEachNode` lambda is already annotated with `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` so all that 's needed is to add `m_connman.m_nodes_mutex` to that. That doesn't work directly, because `m_nodes_mutex` is private and the `ForEachNode` is in a different class, but you can work around that by declaring:

```c++
public:
// alias for thread safety annotations only, not defined.
RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex);
```

in `CConnman`, and writing:
...

👍 ViljoPennanen approved a pull request: "scripted-diff: Specify Python major version explicitly on Windows"
(https://github.com/bitcoin/bitcoin/pull/28213#pullrequestreview-1561825694)
lgtm

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1561722494)
Updated 08f5febfc571220043436bbec96a326beebdee22 -> b0beb4c504da29c27358d4602a045aaab39305f6 ([`pr/bresult2.37`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.37) -> [`pr/bresult2.38`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.38), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.37..pr/bresult2.38)) moving a lot more functionality from the `Result` class to the `ResultBase` class so the new code can be compatible with the `ResultPtr` class from #26022
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283630635)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282487687

> Here and line 192 below: "earning"?

Thanks, rewrote these comments now

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283624276)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282502389

> also, per the lint CI / spelling linter

Thanks, fixed spelling

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283623667)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282503773

> Tidy CI iwyu suggestions

Thanks applied these

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283623420)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282499219

> [584e3fa](https://github.com/bitcoin/bitcoin/commit/584e3fa4ea8ca15b3a8b46e023f218c5e0ed73b0) Not sure, but these `if (!result) return result;` idioms (here and lines 228-229 below) seem "odd" enough that an explanatory comment might be helpful.

I think this will probably be a common pattern and it would be too verbose to explain what is happening each place it is used. If the code were misleading, I think I would w
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1283629856)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282507096

> [6ddcc9b](https://github.com/bitcoin/bitcoin/commit/6ddcc9bec8f64148b7f4b6d03b8dd85a5e748175) `GetErrors()` and `GetWarnings()` don't seem to have any test coverage yet. Note that there is also already a `GetWarnings()` in `src/warnings.{h.cpp}`, perhaps disambiguate naming/grepping-wise in addition to namespace-wise.

Thanks, added some test coverage. On the `warnings.h` overlap, I think that `GetWarnings()` is not
...

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1283703466)
Done

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1283703631)
Done

✅ fanquake closed a pull request: "p2p: diversity network outbounds follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28189)

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1664631599)
Force-pushed:

Addressed:
- https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1272359281
- https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1272371310

Also, I added support for `GetSelectedValue()`, asserting that its value is always greater or equal than `target` in a valid result.

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1283710640)
Does it worth? BnB internally calls `ComputeSetAndWaste` which checks change. Any unexpected action regarding it would affect the assertion - `assert(best_waste == result.GetWaste());`.

💬 brunoerg commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#discussion_r1283716489)
I don't think it's worth to mention this in a release note, seems like a specific code nuance to be exposed. Isn't it?

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1664651486)
> https://github.com/bitcoin/bitcoin/commit/68e48a0185751d24eecb194b8efd7028c8b590f3: can you elaborate a bit in the commit description why nSendOffset can be dropped?

In the current codebase, the send buffering (= remembering the to-be-sent bytes which we haven't managed to send yet) is done `vSendMsg` (a queue of *byte arrays*) + `nSendOffset` (the position within `vSendMsg[0]` up to where we've sent things). After this PR, `vSendMsg` is turned into a queue of *messages*, which have not yet
...

💬 ChristopherA commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664652771)
> > Why do you believe that expanding a publication mechanism that is 4x more expensive than the heavily used taproot witness mechanism will "increase the spam problems significantly"?
>
> @ChristopherA just said that he's paying you to push this because it makes it cheaper and easier for him to write his non-bitcoin stuff into the chain.

Some clarification: I said I would contribute $1K toward a bounty for a real PR. Others said they might contribute to that bounty as well. My real goal w
...

💬 ChristopherA commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664659414)
A question to those who object to this PR, would any of you change your mind if a new `is_standard` was simply a little larger? Maybe a modest 256 or 512 bytes?

Though I believe there are other reasons OP_RETURN shouldn't be limited, if advancing a modest limit is an option, it is at least a way for devs who "want to do the right thing" rather than enormous inscriptions and other hacks, I would support it.

-- Christopher Allen

💬 paplorinc commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1283740534)
```Python
# Test that 1-of-1000 and 999-of-1000 raise exception
for k in [1, 999]:
assert_raises_rpc_error(-5, "Cannot have 1000 keys in multi_a; must have between 1 and 999 keys, inclusive", self.do_test_k_of_n, k, 1000)
```