💬 Krokochik commented on issue "importdescriptors hanging on importing/updating descriptor with large range":
(https://github.com/bitcoin/bitcoin/issues/25895#issuecomment-2059776680)
@willcl-ark no it doesn't

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567845152)
Instead of casting back and forth and having a `GetRealType()` it would, imo, be more straightforward to split the delete flag off from the type in `Unserialize` before casting to `RecordType`. So to have
```
RecordType type;
bool deleted;
```
Then these accessors could go.

🤔 achow101 reviewed a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2004420996)
e0b0a385350d8f7a9d00542cc3d14bb47224d889 "tests: miniscript decaying multisig signing order does not matter" could be squashed into de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig".

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567831190)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

It's not necessary to state where the descriptor comes from.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567833329)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

Using multiple wallets is preferred over using multiple nodes.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567838489)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

A lot of this is very similar to/the same as `wallet_multisig_descriptor_psbt.py`. I would prefer if they were combined into one test file. Note that you can (and should) make separate functions for the different test cases so that there is a distinct separation between them, while still using the same utility functions.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567843544)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

Mining hundreds of blocks can take a while and is unnecessary for this test. This could run quite a bit faster with locktimes that are closer to each other.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567842740)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

This workflow is very similar to what will happen in the loop below. Can they be consolidated?

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567846617)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

This massively overshoots the locktime block each time. I think it would be better to mine just enough blocks to reach the locktime height.

💬 achow101 commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#issuecomment-2059790024)
reACK c6be144c4b774a03a8bcab5a165768cf81e9b06b

💬 pinheadmz commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2059830018)
Thanks for taking a look, @0xB10C. I've refactored the PR to add a new field instead of changing anything already in place. Edited PR title and description. I made the tests super clear that the complete string is returned by `submitrawtransaction` whereas its split into two fields in `testmempoolaccept`

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567887865)
OK I see now thanks. So I guess... `start_node` calls `wait_for_rpc_connection`. Is RPC available before reindexing is finished? If we can rely on that to prevent race conditions then this could just be `assert_debug_log`.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920748)
Good point. Removed this check.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920893)
Added a check

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920972)
Removed

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921413)
Good point. I've added a check earlier that the number of records is even to avoid this kind of issue.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921595)
Done as suggested.

💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568092353)
Instead of hardcoding these new `rpcauth` lines, it might be better to expand the existing `users` list. This would apply to adjacent methods as well.

💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568080142)
This seems to be using the argument `user` as either a string or a list depending on whether or not `password` was provided, which seems less straightforward than using a consistent type. Unless I'm missing something, the `password` argument might not be needed at all, since the `user` list contains the plaintext password. See:

https://github.com/bitcoin/bitcoin/blob/94836b3d96f7fe9ed4225bf7ada961ca5dd554e8/test/functional/rpc_whitelist.py#L40-L45

🤔 tdb3 reviewed a pull request: "Test/rpc whitelistdefault test"
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2004787892)
Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with a security-minded feature.

Concept ACK. Looks like there are opportunities to enhance the existing approach taken. Some inline comments were left.

Built and ran all functional tests (all passed).

Checked that the following comments were addressed from PR #17805.

- The additional tests appear to be integrated into rpc_whitelist.py
- Tests were organ
...