💬 sipa commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1269598813)
I think it would be useful to use a notation like `[ ... x0 x1] -> [... x0 x1 x0 x1]` for these opcodes instead of trying to explain in English.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269613973)
> that'll certainly be faster but then we can't verify a successful reindex in the new run-as-root path.

Any reason why it wouldn't be able to reindex?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1644114670)
lgtm ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6 🥛

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 08b275c6d27f06cd
...

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