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

💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568071896)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

>Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.

💬 tdb3 commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1568068748)
nit: See https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

>If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts.

💬 laanwj commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060364162)
i'm divided on this conceptually, i think with the reputation it has now, another successful attack on xz specifically is unlikely (not more likely than any other library, at least). And i'm not sure we need to go out of our way to avoid it given how much of the FOSS ecosystem relies on it.

One thing i am slightly afraid of is that in avoiding xz we become more sloppy about dependencies in other ways. Not saying that that is happening here.

But e.g. not being able to use the primary source
...

💬 laanwj commented on pull request "test: Fix failing univalue float test":
(https://github.com/bitcoin/bitcoin/pull/29892#issuecomment-2060405735)
Right. Direct floating point equality checks are a big nono. It should either use an epsilon comparison, or discretize. Which is what the new test does.

ACK fa4c69669e079c38844ecea1ad3394aae3702ae1

💬 maflcko commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060446429)
Not sure about the benefit to make extra steps to use a possibly non-standard, possibly easier to backdoor, mirrored tarball.

An alternative might be to completely skip the step of tarballs and instead use the upstream source control directly?