19:00:09 <wumpus> #startmeeting 19:00:09 <core-meetingbot> Meeting started Thu Feb 25 19:00:09 2021 UTC. The chair is wumpus. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings. 19:00:09 <core-meetingbot> Available commands: action commands idea info link nick 19:00:16 <achow101> hi 19:00:20 <hebasto> hi 19:00:22 <amiti> hi 19:00:27 <wumpus> #bitcoin -core-dev Meeting: achow101 aj amiti ariard bluematt cfields Chris_Stewart_5 digi_james dongcarl elichai2 emilengler fanquake fjahr gleb glozow gmaxwell gwillen hebasto instagibbs jamesob jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack jtimon kallewoof kanzure kvaciral lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball morcos nehan NicolasDorier paveljanik 19:00:29 <wumpus> petertodd phantomcircuit promag provoostenator ryanofsky sdaftuar sipa vasild wumpus 19:00:44 <meshcollider> hi 19:00:59 <wumpus> it doesn't look like any topics have been proposed this week, any last minute ones ? 19:01:01 <sipa> hi 19:01:09 <sdaftuar> hi 19:01:09 <fjahr> hi 19:01:33 <wumpus> (fwiw, if you want to propose a meeting topic during the week use #proposedmeetingtopic <topic> and it will appear in http://gnusha.org/bitcoin-core-dev/proposedmeetingtopics.txt) 19:02:15 <wumpus> #topic High priority for review 19:02:15 <core-meetingbot> topic: High priority for review 19:02:31 <wumpus> https://github.com/bitcoin/bitcoin/projects/8 7 blockers, 2 chasing concept ACK 19:02:52 <wumpus> congrats to provoostenator for getting the external wallet signer merged 19:03:02 <amiti> can I add #21061 to the list? 19:03:02 <wumpus> anything to add/remove or ready for merge? 19:03:04 <gribble> https://github.com/bitcoin/bitcoin/issues/21061 | [p2p] Introduce node rebroadcast module by amitiuttarwar ÷ Pull Request #21061 ÷ bitcoin/bitcoin ÷ GitHub 19:03:28 <fjahr> I would like to add #19521 19:03:32 <gribble> https://github.com/bitcoin/bitcoin/issues/19521 | Coinstats Index by fjahr ÷ Pull Request #19521 ÷ bitcoin/bitcoin ÷ GitHub 19:03:44 <wumpus> amiti: added! 19:03:49 <amiti> wumpus: thank you :) 19:04:01 <achow101> I'd like to replace #17331 with #21083 19:04:07 <gribble> https://github.com/bitcoin/bitcoin/issues/17331 | Use effective values throughout coin selection by achow101 ÷ Pull Request #17331 ÷ bitcoin/bitcoin ÷ GitHub 19:04:08 <gribble> https://github.com/bitcoin/bitcoin/issues/21083 | wallet: Avoid requesting fee rates multiple times during coin selection by achow101 ÷ Pull Request #21083 ÷ bitcoin/bitcoin ÷ GitHub 19:04:12 <jonatack> hi 19:04:18 <wumpus> fjahr: also added 19:04:21 <fjahr> wumpus: thanks! 19:05:04 <wumpus> achow101: done 19:05:11 <jnewbery> hi 19:05:18 <jamesob> hi 19:06:07 <wumpus> anything else for this topic? any other topics to discuss? 19:07:07 <achow101> topic suggestion: testing strategy for descriptor and legacy wallets 19:07:08 <emzy> hi 19:07:15 <achow101> (perhaps this is more suited for the wallet meeting next week) 19:07:26 <wumpus> achow101: okay, up to you 19:07:50 <achow101> if there's nothing else to discuss 19:07:54 <wumpus> i'm fine with either, we can discuss it now otherwise it's the end of the meeting 19:07:55 <wumpus> ok 19:08:07 <wumpus> #topic Testing strategy for descriptor and legacy wallets (achow101) 19:08:07 <core-meetingbot> topic: Testing strategy for descriptor and legacy wallets (achow101) 19:08:41 <achow101> Currently I find the way that we do tests for both descriptor and legacy wallets a bit halfassed and not that great 19:09:04 <achow101> so I had opened #20892 which runs both descriptor and legacy wallet variants of a test at a time 19:09:06 <gribble> https://github.com/bitcoin/bitcoin/issues/20892 | tests: Run both descriptor and legacy tests within a single test invocation by achow101 ÷ Pull Request #20892 ÷ bitcoin/bitcoin ÷ GitHub 19:09:17 <achow101> however ryanofsky disagrees with this approach 19:09:49 <achow101> what other options or suggestions do people have for running tests that should be run on both descriptor and legacy wallets? 19:09:59 <wumpus> if the parts are completely independent, why not split it up 19:10:13 <achow101> the particular issue is with tests that should be run on both 19:10:33 <wumpus> e.g. have the test run with --legacy and --descriptor-wallet based on what is enabled 19:10:42 <wumpus> as separate invocations 19:10:49 <luke-jr> isn't that how it works now? 19:10:53 <achow101> that's how it works how 19:11:01 <wumpus> what's wrong with that then ? 19:11:20 <achow101> but if I am running a test without the test runner, that's 2 commands and not everyone who is working on wallet stuff may realize that 19:11:32 <wumpus> well add a --both for that maybe? 19:11:48 <wumpus> for manual use 19:11:56 <luke-jr> default being both seems fine for manual use 19:12:00 <wumpus> yes 19:12:04 <luke-jr> test runner can still specify them separately 19:12:27 <achow101> that seems reasonable 19:12:30 <wumpus> i mean if it saves you hassle during development (don't forget to document it) then why not 19:13:20 <achow101> there are also a number of test cases within some of the tests that are wallet type specific that seem like there should be an indicator of being skipped 19:13:38 <achow101> but I think it would be better to split those into separate tests, although that is a non trivial task 19:13:48 <jnewbery> I think ideally we only want this behavoiur for wallet_ tests (tests that are explicitly testing the wallets), and not the tests that incidentally use the wallet for doing things like constructing transactions 19:13:50 <luke-jr> achow101: eh, we don't "skip" tests that don't make sense :P 19:13:58 <wumpus> that's why i prefer the split approach for the test runner, it's easier to indicate what is skipped 19:14:02 <luke-jr> feature_namecoin_integration [SKIP] 19:14:04 <luke-jr> :P 19:14:26 <achow101> jnewbery: yes. we should encourage MiniWallet use 19:14:45 <wumpus> jnewbery: agree, we don't want to end up running everything twice if the point is not the wallet 19:15:08 <achow101> the problem with some tests incidentally using the wallet is that determining the type of wallet to create in that case is pretty annoying since we have to take into account what wallet support is compiled 19:15:28 <luke-jr> achow101: the framework can deal with that easily enough 19:15:28 <jnewbery> yes, we should, but there are still tests that use the wallet incidentally and it'd be a shame to make them run for twice as long in the mean time 19:15:35 <achow101> luke-jr: it's a bit messy 19:15:45 <luke-jr> why? 19:16:01 <wumpus> maybe add a preferred_oneoff_wallet_type() or so 19:16:15 <achow101> as in, the logic currently for dealing with that is a bit messy imo 19:16:17 <luke-jr> incidental use shouldn't care which kind it gets 19:16:37 <wumpus> (until it's all switched to MiniWallet ofc) 19:17:26 <sipa> are there any blockers for miniwallet to be used more? or is it just the effort of transitioning the tests to use it? 19:17:44 <wumpus> I think the bottleneck is review 19:17:49 <sipa> of course. 19:17:58 <wumpus> there's a lot of PRs open that convert tests to miniwallet IIRC 19:18:08 <achow101> there are a surprising number of tests that use the wallet incidentally 19:18:27 <wumpus> all the "run XXX even with wallet disabled" PRs 19:19:21 <sipa> i was thinking about moving the generic signing framework in feature_taproot to be in the test_framework and making it more generally usable (as it has rough code for signing basically anything) 19:19:34 <luke-jr> what is wrong with using the wallet incidentally? just that it means we don't test it on non-wallet builds? 19:19:44 <wumpus> the issue for this is #20078 19:19:46 <gribble> https://github.com/bitcoin/bitcoin/issues/20078 | test: Convert non-wallet tests to use our python MiniWallet ÷ Issue #20078 ÷ bitcoin/bitcoin ÷ GitHub 19:20:03 <jnewbery> At some point in the future (perhaps when multiprocess is merged), would it make sense for the wallet_ tests to not use the full test framework, but instead to have the wallet interact with a mock node? 19:20:07 <wumpus> luke-jr:that's another advantage yes 19:20:37 <achow101> luke-jr: depending on the test, sometimes there is no problem, and other times the test relies on some specific behavior of the wallet that is not guaranteed across types 19:20:59 <achow101> e.g. there's a test for doing something with conflicts which relies on sethdseed which is not available for descriptor wallets 19:21:54 <achow101> that particular test isn't doing anything with the wallet except needing the same privkeys on 2 nodes 19:22:00 <luke-jr> XD 19:23:17 <achow101> in any case, it seems like i should keep the test runner as it is and just make the run twice behavior for manual test invocation 19:23:41 <jnewbery> only for wallet_ tests 19:23:45 <wumpus> right the two wallets are not entirely interchangable and that can result in ugliness 19:23:49 <achow101> yes, only for wallet tests 19:23:57 <jnewbery> SGTM 19:24:14 <wumpus> SGTM too 19:24:26 <luke-jr> wonders how difficult it would be at this point to add a new wallet type that is just a wrapper around an external Lightning wallet 19:24:41 <meshcollider> ð 19:25:56 <wumpus> any other topics? 19:26:50 <wumpus> #endmeeting