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

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