💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1265311577)
Needs rebase

👍 0xB10C approved a pull request: "test: Disable known broken USDT test"
(https://github.com/bitcoin/bitcoin/pull/28088#pullrequestreview-1532737766)
ACK on disabling the test for now - I'll revisit #27380 when I have more bandwith it.

🤔 MarcoFalke reviewed a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1527628367)
Sure, happy to leave comments, but I think it may be better to fix them in a follow-up, unless you really want to here.

💬 MarcoFalke commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1262023316)
```suggestion
BOOST_CHECK_EQUAL(HexStr(total_tag), "64afe2e8d6ad7bbdd287f97c44623d39");
```

style nit, if you retouch.

💬 MarcoFalke commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1265337111)
nit: If you want to avoid the cast here, you could make `ConsumeRandomLengthByteVector` a template on `B` and then directly consume into a `std::vector<std::byte>` and use that.

💬 MarcoFalke commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1265338188)
```suggestion
std::vector<std::byte> msg(i, std::byte{uint8_t(i)});
```

(nit, just personal preference)

💬 furszy commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1265351007)
Guess that you are referring to the last two args?

I think that it isn't much of a problem because it is an internal static helper function.

They are needed for Knapsack min change target calculation. What maybe could do is to pass them as one: `mean_amount = std::floor(recipients_sum / recipients_size)` and document the function properly.

And also, in a follow-up, we could go further and move the else branch of the `scriptChange` block of code inside `ComputeChangeParams` too. That par
...

📝 MarcoFalke opened a pull request: "test: Bump walletpassphrase timeout to avoid intermittent issue"
(https://github.com/bitcoin/bitcoin/pull/28089)
There is no risk increasing the timeout, but it avoids intermittent issues (when running in valgrind) of the form:

```
node1 2023-07-15T06:17:37.827609Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:45950
node1 2023-07-15T06:17:37.833168Z [httpworker.3] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=signrawtransactionwithwallet user=__cookie__
test 2023-07-15T06:17:38.037000Z TestFramework (ERROR): JSONRPC error

...

📝 fanquake opened a pull request: "validation: use noexcept instead of deprecated throw()"
(https://github.com/bitcoin/bitcoin/pull/28090)
We fixed this once before in https://github.com/bitcoin/bitcoin/pull/10965.
Turn on https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-noexcept.html#modernize-use-noexcept.

💬 fanquake commented on pull request "test: Disable known broken USDT test":
(https://github.com/bitcoin/bitcoin/pull/28088#issuecomment-1638210191)
Seems fine to disable this for now.
Also cc @virtu.

🚀 fanquake merged a pull request: "test: Disable known broken USDT test"
(https://github.com/bitcoin/bitcoin/pull/28088)

💬 furszy commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265432942)
This looks fishy. It seems that we unlock the wallet inside a specific test case, and use the unlocked period for other test cases.

A better approach would be to relock the wallet at the end of this test case ( `self.nodes[1].walletlock()`). And unlock-lock the wallet in the other cases when it is needed.

💬 willcl-ark commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1265436928)
Ok I've used a `GlobalMutex` for this now. I never Recursive behaviour but was getting clang static analysis errors without. Looking at _sync.h_ it seems that `GlobalMutex` should be the correct type for this I think.

💬 MarcoFalke commented on pull request "test: Bump walletpassphrase timeout to avoid intermittent issue":
(https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265441187)
Sounds good. An alternative might be to not use a passphrase after this test. Do you want to submit either as a pull request? Happy to close mine then. Otherwise, I'll leave the feedback for a follow-up, as the goal here is to not change any logic, only the constant.

💬 willcl-ark commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1265441313)
`DeleteNode()` now gated behind `if (pnode->nRefCount.compare_exchange_strong(expectedRefCount, -1))` so it can only be called once (by a single thread).

💬 MarcoFalke commented on pull request "validation: use noexcept instead of deprecated throw()":
(https://github.com/bitcoin/bitcoin/pull/28090#issuecomment-1638254580)
lgtm ACK 047daad4f59942488163c6be8516a69291646294

📝 brunoerg opened a pull request: "fuzz: use `ConnmanTestMsg` in `connman`"
(https://github.com/bitcoin/bitcoin/pull/28091)
Fixes #27980

Using `ConnmanTestMsg` we can add nodes and be
more effective fuzzing functions like `DisconnectNode`,
`FindNode`, `GetNodeStats` and other ones.

📝 fanquake opened a pull request: "build: document that -Wreturn-type has been fixed upstream (mingw-w64)"
(https://github.com/bitcoin/bitcoin/pull/28092)
`noreturn` attributes have been added to the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, meaning that [from 11.0.0 onwards](https://www.mingw-w64.org/changelog/), you'll no-longer see `-Wreturn-type` warnings when using `assert(false)`.

At the same time, move this out of the `-Werror` check, it's not clear why it was in there, rather than being applied/or-not along with all other cxxflags. A somewhat similar change was proposed in
...

💬 fanquake commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1638262812)
> but I think having a macro for those that find it helpful, can't hurt, can it?

I agree that having a macro could be useful.

Separately, note that upstream, the missing `noreturn` attributes have been added to `assert` in the mingw-w64 headers, https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe, so beginning with [11.0.0](https://www.mingw-w64.org/changelog/), there are no-longer `-Wreturn-type` warnings with `assert(false)` usage. See #28092 where I've
...

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265452083)
I haven't done a careful review of all the places we use `IsInitialBlockDownload()` to confirm this, but my intuition would be that if we're using an assumeutxo snapshot, what we care about is whether our snapshot-based chainstate is caught up or not -- basically by definition, the background chainstate will always be "in initial block download" until it finishes and is no longer in use, so it wouldn't make sense to condition anything on whether it's behind or not.

So yeah I think moving this
...