👍 ryanofsky approved a pull request: "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count"
(https://github.com/bitcoin/bitcoin/pull/27511#pullrequestreview-1539500332)
Code review ACK 7d7ee5e0f4254495349ea68f3d0127c0c336fd12

💬 ryanofsky commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269601639)
In commit "rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table" (332cf61a5cdf838a20117280dc2d24659a4b4a5c)

It seems inaccurate to say "RPC is for testing only" when PR description says "This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman" which makes it sound like this is useful for troubleshooting or monitoring.

If the RPC is not intended just to test the software, I think it
...

🚀 achow101 merged a pull request: "descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067)

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269620812)
ah ya know what I didn't read your comment correctly I thought you were suggesting filling the blocks with invalid junk but that line with `generateblock()` is cool

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261648754)
hmmm, this is a `txid`, not a `wtxid`, which is essentially a no-op unless it's a non-segwit tx. Could we instead call `MempoolRejectedTx` at the point where `MEMPOOLREJ` logging is done, when we have access to the wtxid, since that's the case we actually care about?

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1269622220)
also, adding wtxid logging at `MEMPOOLREJ` log is done would be <3

💬 instagibbs commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1644129957)
concept ACK, it comes up pretty [often](https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261648754)

⚠️ darosior opened an issue: "fuzz: miniscript: test the node is satisfiable before dereferencing `GetStackSize`"
(https://github.com/bitcoin/bitcoin/issues/28114)
After #27997. Mostly a self-reminder that the fuzz target could crash at any moment on master on OSS fuzz. I'll do it tomorrow, hopefully Marco won't come up with an OSS fuzz bug report before then.

💬 MarcoFalke commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644132699)
Maybe every `class` other than `CWallet` can be moved to a separate module?


Also, a bunch of includes can be killed, according to iwyu:

```
wallet/wallet.h should remove these lines:
- #include <interfaces/wallet.h> // lines 12-12
- #include <psbt.h> // lines 16-16
- #include <util/message.h> // lines 20-20
- #include <util/strencodings.h> // lines 22-22
- #include <validationinterface.h> // lines 26-26
- #include <wallet/walletdb.h> // lines 30-30
- #include <algorithm>
...

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1644136991)
Rebased (and corrected a comment in the fuzz test witness padding commit).

💬 achow101 commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644144474)
ACK e667bd68a10512ddc784df44bdcb63ee441e5551

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269641610)
It is already ugly to have a timeout in the functional tests, and use it to sync them. (At least this one is std::optional)

However, my preference would still be to not have this and instead force the caller to sync.

Otherwise this will just lead to brittle code down the line and potentially intermittent test issues when running with libc++ sanitizers in valgrind on arm64, etc.

💬 MarcoFalke commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644149971)
Heh, if there's a known bug that should be found by fuzzers, but isn't, then the tests should be fixed to find it :)

💬 jonatack commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1644155681)
@miketwenty1 Needs rebase. Are you still working on this?

✅ achow101 closed an issue: "failure in wallet_resendwallettransactions.py --legacy-wallet"
(https://github.com/bitcoin/bitcoin/issues/28094)

🚀 achow101 merged a pull request: "test: fix intermittent failure in wallet_resendwallettransactions.py"
(https://github.com/bitcoin/bitcoin/pull/28108)

⚠️ jonatack opened an issue: "Win64 CI failure in feature_versionbits_warning.py"
(https://github.com/bitcoin/bitcoin/issues/28115)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

https://cirrus-ci.com/task/6148314429652992?logs=functional_tests#L2861

### Expected behaviour

Expect CI to pass for unrelated changes.

### Steps to reproduce

https://cirrus-ci.com/task/6148314429652992?logs=functional_tests#L2861

### Relevant log output

```
test 2023-07-19T19:45:37.140000Z TestFramework (ERROR): Assertion failed
Traceback (mos
...

💬 theuni commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644163497)
I don't see any reason not to do the iwyu trimming. Though sadly it doesn't look like it'll help too much. Preprocessed (gcc -E -O0) numbers for `wallet/wallet.cpp` before and after @MarcoFalke's suggested removals:
```
Before: 191,084 lines
After: 190,899 lines
```

💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644167053)
Try this:

```diff
--- a/src/crypto/sha256_sse4.cpp
+++ b/src/crypto/sha256_sse4.cpp
@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)

"Ldone_hash_%=:"

- : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+m"(xfer)
+ : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d
...

💬 sipa commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644175785)
There is no requirement that an entire class is defined within a single compilation unit, I believe. If you don't have function definitions inside the `class` itself, then those definitions can be given in any .cpp file.

That said, for understandability I'd strongly prefer not splitting a class over multiple .cpp files...