< vasild>
thanks! unrelated to that, I got to #20197 in my to-(re)review list :)
< gribble>
https://github.com/bitcoin/bitcoin/issues/20197 | p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack · Pull Request #20197 · bitcoin/bitcoin · GitHub
< vasild>
hm, since you mentioned the I2P PR, I have been thinking about adding fuzz tests to it and I think it will be necessary to change the data members from Sock to Sock* - will be purely mechanical if needed
< vasild>
Sock* or std::unique_ptr<Sock>
< vasild>
this is because right now a test cannot "replace" the Sock member with FuzzedSock (which inherits Sock)
< jonatack>
vasild: thanks! above all, looking for feedback on the protection allocation ratios between regular, onion, localhost (and soon i2p, and maybe cjdns) peers, i went for the simplest thing but it maybe protects too many localhost and onion peers
< vasild>
but if the main code uses pointers and a global function "create a Sock for me and give me a pointer to it", then a test could replace that global function with another one which returns a pointer to FuzzedSock
< vasild>
(I did not test the above but am 99% sure that this will be the case)
< vasild>
wumpus: ^
< jonatack>
vasild: no worries. do you think it should be done in 20685?
< vasild>
wumpus: since you ACK'ed it already, just a headsup that maybe a Sock -> Sock* mechanical change is coming in order to accomodate fuzzing :)
< vasild>
not strictly necessary, but would be good to avoid a followup PR, will see once I have the code
< jonatack>
diff shouldn't be too bad if mechanical
< vasild>
yes, I imagine something like s/Sock/std::unique_ptr<Sock>/ and s/sock./sock->/ and a difference when constructing Sock objects
< 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
< jonatack>
hi
< wumpus>
fjahr: also added
< fjahr>
wumpus: thanks!
< wumpus>
achow101: done
< jnewbery>
hi
< jamesob>
hi
< wumpus>
anything else for this topic? any other topics to discuss?
< achow101>
topic suggestion: testing strategy for descriptor and legacy wallets
< emzy>
hi
< achow101>
(perhaps this is more suited for the wallet meeting next week)
< wumpus>
achow101: okay, up to you
< achow101>
if there's nothing else to discuss
< wumpus>
i'm fine with either, we can discuss it now otherwise it's the end of the meeting
< wumpus>
ok
< wumpus>
#topic Testing strategy for descriptor and legacy wallets (achow101)
< core-meetingbot>
topic: Testing strategy for descriptor and legacy wallets (achow101)
< achow101>
Currently I find the way that we do tests for both descriptor and legacy wallets a bit halfassed and not that great
< achow101>
so I had opened #20892 which runs both descriptor and legacy wallet variants of a test at a time
< 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
< achow101>
however ryanofsky disagrees with this approach
< achow101>
what other options or suggestions do people have for running tests that should be run on both descriptor and legacy wallets?
< wumpus>
if the parts are completely independent, why not split it up
< achow101>
the particular issue is with tests that should be run on both
< wumpus>
e.g. have the test run with --legacy and --descriptor-wallet based on what is enabled
< wumpus>
as separate invocations
< luke-jr>
isn't that how it works now?
< achow101>
that's how it works how
< wumpus>
what's wrong with that then ?
< 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
< wumpus>
well add a --both for that maybe?
< wumpus>
for manual use
< luke-jr>
default being both seems fine for manual use
< wumpus>
yes
< luke-jr>
test runner can still specify them separately
< achow101>
that seems reasonable
< wumpus>
i mean if it saves you hassle during development (don't forget to document it) then why not
< 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
< achow101>
but I think it would be better to split those into separate tests, although that is a non trivial task
< 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
< luke-jr>
achow101: eh, we don't "skip" tests that don't make sense :P
< wumpus>
that's why i prefer the split approach for the test runner, it's easier to indicate what is skipped
< luke-jr>
feature_namecoin_integration [SKIP]
< luke-jr>
:P
< achow101>
jnewbery: yes. we should encourage MiniWallet use
< wumpus>
jnewbery: agree, we don't want to end up running everything twice if the point is not the wallet
< 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
< luke-jr>
achow101: the framework can deal with that easily enough
< 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
< achow101>
luke-jr: it's a bit messy
< luke-jr>
why?
< wumpus>
maybe add a preferred_oneoff_wallet_type() or so
< achow101>
as in, the logic currently for dealing with that is a bit messy imo
< luke-jr>
incidental use shouldn't care which kind it gets
< wumpus>
(until it's all switched to MiniWallet ofc)
< sipa>
are there any blockers for miniwallet to be used more? or is it just the effort of transitioning the tests to use it?
< wumpus>
I think the bottleneck is review
< sipa>
of course.
< wumpus>
there's a lot of PRs open that convert tests to miniwallet IIRC
< achow101>
there are a surprising number of tests that use the wallet incidentally
< wumpus>
all the "run XXX even with wallet disabled" PRs
< 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)
< luke-jr>
what is wrong with using the wallet incidentally? just that it means we don't test it on non-wallet builds?
< 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?
< wumpus>
luke-jr:that's another advantage yes
< 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
< achow101>
e.g. there's a test for doing something with conflicts which relies on sethdseed which is not available for descriptor wallets
< achow101>
that particular test isn't doing anything with the wallet except needing the same privkeys on 2 nodes
< luke-jr>
XD
< 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
< jnewbery>
only for wallet_ tests
< wumpus>
right the two wallets are not entirely interchangable and that can result in ugliness
< achow101>
yes, only for wallet tests
< jnewbery>
SGTM
< wumpus>
SGTM too
< * 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