👍 dergoegge approved a pull request: "fuzz: set `nMaxOutboundLimit` in connman target"
(https://github.com/bitcoin/bitcoin/pull/29172#pullrequestreview-1806452323)
utACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46

🤔 stickies-v reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1805846181)
Approach ACK, nice one finding this test case! 1a52ca7619eeb1baafa5a32b364381126862b29d looks good to me but not super familiar with this part of the code. Left some suggestions about maintainability and can be ignored.

I verified that the tests fail on #29019 as well as on 453b4813ebc74859864803e9972b58e4be76a4d6~1, as is necessary.

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1442918332)
nit: makes more sense to unpack here imo
```suggestion
block_to_disconnect = self.generate(self.nodes[0], 1)[0]
```

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1442808773)
nit: could be a bit more efficient by only calling and iterating over `listunspent` once (does not test for txid collisions, but I don't think we need that?)

<details>
<summary>git diff on 1a52ca7619</summary>

```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..928455c355 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -286,13 +286,12 @@ class ImportRescanTest(Bi
...

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1443144137)
It doesn't seem like we're actually testing the expected balance at any point, rather the amount received, but I may be missing nuance. Would this diff make sense? I don't like adding an extra parameter to an already long function for something so specific, if we can avoid it.

<details>
<summary>git diff on 1a52ca7619</summary>

```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..fa9f43f66c 100755
--- a/test/functional
...

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1443052612)
nit: I don't think we're actually checking the confirmations values, so for future maintenance, would this be more straightforward?
```suggestion
variant.node.gettransaction(variant.initial_txid) # raises if tx wasn't rescanned
variant.node.gettransaction(variant.child_txid) # raises if tx wasn't rescanned
```

💬 Christewart commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1879019144)
> Ok, it is called, but you also have to wait for it to complete and return from the RPC, before continuing the test. Currently it looks like it is called, and processes the block events, but at the same time the P2P interface is asked for the filters.
>
> It may be best to only use a single thread for the test logic.

This is the behavior of this code snippet in Scala

https://github.com/bitcoin-s/bitcoin-s/blob/ff8376ceb664436e9d56da02b1f7598da1dbc459/bitcoind-rpc/src/main/scala/org/bit
...

💬 dongcarl commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG` & native_llvm":
(https://github.com/bitcoin/bitcoin/pull/29188#issuecomment-1879020304)
Concept ACK

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879020324)
Force-pushed: Rebased and addressed https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365833858 and https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1440760716 (I moved the `-whitelistrelay` and `-whitelistforcerelay` verification to the `AddWhitelistPermissionFlags` function.

💬 dergoegge commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879021930)
> It's meant to limit extra data in transactions. OP_RETURN was supposed to be the only tolerated way to do that.

Can you add any references for this? because the GitHub and git history very clearly contradicts your statements (https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1878972398).

💬 theuni commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1879022696)
Nice use of tidy!

Concept ACK, assuming `::contains()` exists for common stdlibs by now. Waiting to see what c-i thinks.

💬 Christewart commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1879024752)
> Ok, it is called, but you also have to wait for it to complete and return from the RPC, before continuing the test. Currently it looks like it is called, and processes the block events, but at the same time the P2P interface is asked for the filters.
>
> It may be best to only use a single thread for the test logic.

Hmm, you are totally right. You said elsewhere in this thread

>Even when the peer sent the headers message, the block filter index could still be processing the block sign
...

💬 1thales commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879025018)
Ping @Davidson-Souza

💬 stickies-v commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879026448)
Concept ACK

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1443157721)
Good point; I'll add a check and a comment.

💬 luke-jr commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879034293)
No, it doesn't contradict it. That overview conveniently leaves out the context of OP_RETURN being the _only_ way tolerated, and the `datacarriersize` limit makes no sense the way you want to spin it.

💬 achow101 commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1443161309)
In 559529480fedd4181dab76f4452b5998d0cdadc3 "refactor: decouple wallet EraseTxns from ZapSelectTx"

Unnecessary whitespace change.

💬 achow101 commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1443160561)
In 559529480fedd4181dab76f4452b5998d0cdadc3 "refactor: decouple wallet EraseTxns from ZapSelectTx"

The commit message calls this `EraseTxns`.

💬 achow101 commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1443164969)
In ff214259de28216a5190c358b786306985f30227 "wallet: migration, remove watch-only transactions atomically"

It seems unnecessary to be making this change here as `ZapSelectTx` is now atomic.

🚀 fanquake merged a pull request: "wallettool: Always be able to dump a wallet's database"
(https://github.com/bitcoin/bitcoin/pull/29117)