👍 brunoerg approved a pull request: "wallet: refactor: remove unused `SignatureData` instances in spkm's `FillPSBT` methods"
(https://github.com/bitcoin/bitcoin/pull/28833#pullrequestreview-1865444517)
crACK e2ad343f69af4f924b22dccf94a52b6431ef6e3c

I checked that `FillSignatureData` only modifies `FillSignatureData` and does not affect anything from the `PSBTInput` itself. Since `SignatureData` instances are not used in spkm's `FillPSBT`methods, makes sense to remove them. Also, `FillSignatureData` could return `SignatureData` instead of void.

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1929956244)
Rebased. This has failed CI once when waiting for the disconnection after:

```
cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
```
I've tried to reproduce it but I cannot in my local setup, the only thing that comes to mind is that mock time may be too close to the timeout (only one second ahead), so I gave it a delta of 10 seconds instead

💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1479995161)
Done in 25dc427e6c755b4c0d68599966caf95ada3565f7 , thank you for the review.

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1480042381)
Either sounds fine! The main point is we shouldn't be removing test cases for maxtxfee while adding maxtxfeerate.

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1480054753)
you can follow the conversation under the issue: https://github.com/bitcoin/bitcoin/issues/27593

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1480059345)
@maflcko `extra_args` parameter cannot be removed since it's a named parameter, unless the `TestNode.__init__(...)` signature is changed

👋 BrandonOdiwuor's pull request is ready for review: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/29387)

💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1480069074)
Why not add this to create_cache.py properly, like in all other wallet tests?

💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-1930102867)
I believe this CI failure is spurious: https://github.com/bitcoin/bitcoin/pull/29371/checks?check_run_id=21277712706

Could someone restart the job (:pray:) or I can push an empty commit to restart everything.

💬 epiccurious commented on pull request "wallet: remove unused 'accept_no_keys' arg from decryption process":
(https://github.com/bitcoin/bitcoin/pull/29375#issuecomment-1930113048)
utACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452.

👍 theStack approved a pull request: "wallet: remove unused 'accept_no_keys' arg from decryption process"
(https://github.com/bitcoin/bitcoin/pull/29375#pullrequestreview-1865757644)
Code-review ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452

💬 mzumsande commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1480178708)
I think it would be nice to have consistency among `-checkaddrman`, `-checkmempool` and `-checkblockindex`. Since non-determinism would only occur for values other than 0 and 1, maybe it'd be best not to use other values than that in tests? Or change all three do be deterministic?

💬 ryanofsky commented on pull request "init: settings, do not load auto-generated warning msg":
(https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1480187026)
re: https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1465379877

> As the settings map take strings for keys, wouldn't this change mean that we would need to static cast string_view to string to not create a copy during the push/erase methods call?

The idea is that just that it is noticeable when programs are slow to start, so you usually want to reduce the amount of code that needs to run before main(), even if it means code that runs later but is less sensitive might be slower. I
...

💬 josibake commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190)
Concept ACK

Started reviewing, looks good. Will continue review tomorrow. Might be worth updating the last part of the description with the insights from https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1874716791 ?

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480260826)
@murchandamus Let me elaborate for context, but no point in addressing this in this PR (after my edit, I don't think it's a particularly impactful improvement even).

Let's say the transactions, in order after sorting are A,B,C,D,E,F. Imagine we've just processed `_BC`, and SHIFT after it (so, without clone skipping, the next exploration would be `_B_D`). If it happens to be the case that D is unambiguously worse than C, then D can be skipped as well. This happens when value(D)=value(C) AND we
...

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1480301257)
Moved it.

💬 achow101 commented on pull request "wallet: remove unused 'accept_no_keys' arg from decryption process":
(https://github.com/bitcoin/bitcoin/pull/29375#issuecomment-1930470491)
ACK 2bb25ce5023c4d56c8b11e9c75f9f8bd69894452

👍 theStack approved a pull request: "test: Fix CPartialMerkleTree.nTransactions signedness"
(https://github.com/bitcoin/bitcoin/pull/29363#pullrequestreview-1865991242)
LGTM ACK facafa90f7a1eee452f9baf8a1b65a2edac0982b

📝 brunoerg opened a pull request: "i2p: log connection was refused due to arbitrary port"
(https://github.com/bitcoin/bitcoin/pull/29393)
For I2P, we do not try to connect if port is != 0. However, we do not have anything that indicates it or any error when trying to connect with port != 0. This PR adds a log for it. Also, it improves the functional test. With this log we can ensure the reason we won't connect is the port, in the current test, we cannot ensure it.

🚀 achow101 merged a pull request: "wallet: remove unused 'accept_no_keys' arg from decryption process"
(https://github.com/bitcoin/bitcoin/pull/29375)