💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478371817)
Thanks, good catch

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478375389)
Added

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478399611)
I had also set it for BnB originally, but then I got the complaint that it wasn't being read anywhere yet. I’ll add it when I update BnB accordingly.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478371586)
Sure, added the mention of that.

💬 delta1 commented on pull request "test: speedup bip324_cipher.py unit test":
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1929792710)
> Why did you choose this approach rather than adding a seek/skip_packet method?

@epiccurious he explained in the very next line:

> that seemed overkill to me only for speeding up a unit test. Open for suggestions

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1479906114)
Fair

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1929818726)
@maflcko this is because [`self.options.descriptors`](https://github.com/bitcoin/bitcoin/blob/4de84557d6d1f53255ab19f27c8b6ea0a712934a/test/functional/test_framework/test_node.py#L111-L112) == None in `TestNode(... , descriptors=self.options.descriptors, ...)` hence the `disable_wallet` flag is added when the TestNode is instantiated disabling the wallet RPCs just before the first [`start_node()`](https://github.com/bitcoin/bitcoin/blob/4de84557d6d1f53255ab19f27c8b6ea0a712934a/test/functional/te
...

💬 epiccurious commented on pull request "test: speedup bip324_cipher.py unit test":
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1929869880)
Tested ACK a8c3454ba137dfac20b3c89bc558374de0524114.

Before (commit 9eeee7caa3f95ee17a645e12d330261f8e3c2dbf):
```
Ran 2 tests in 15.541s
```

After:
```
Ran 2 tests in 0.204s
```

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