< bitcoin-git>
bitcoin/master fadc08a Andrew Chow: Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman
< bitcoin-git>
bitcoin/master eb81fc3 Andrew Chow: Refactor: Allow LegacyScriptPubKeyMan to be null
< bitcoin-git>
[bitcoin] meshcollider merged pull request #17261: Make ScriptPubKeyMan an actual interface and the wallet to have multiple (master...wallet-box-pr-2) https://github.com/bitcoin/bitcoin/pull/17261
< meshcollider>
achow101 ^
< achow101>
\o/
< achow101>
finally
< aj>
did travis not run for 17261 ? looks like it missed a unique_ptr<SigningProvider> in wallet/test/psbt_wallet_tests.cpp
< fanquake>
Probably a merge issue.
< fanquake>
Which is concerning given it got a tested ACK
< fanquake>
Hard to test something that doesn't compile
< aj>
ah, the PR introducin the test also only just got merged
< aj>
so the PRs independently were okay, didn't conflict to cause a rebase, and the merge wasn't tested
< aj>
or not
< aj>
no psbt_wallet_tests was part of this pr
< bitcoin-git>
[bitcoin] ajtowns opened pull request #18026: psbt_wallet_tests: use unique_ptr for GetSigningProvider (master...202001-getsigningprovider-fix) https://github.com/bitcoin/bitcoin/pull/18026
< fanquake>
I'm seeing the failure locally now as well. So I assume the merge script was run, but nothing was actually compiled and no tests were run before signing off.
< aj>
it was #17156 that conflicted with 17261 by the looks
< fanquake>
Yea looks like it. I'll test and merge the fixup in a few.
< * fanquake>
aj are the functional tests broken as well? I see at least one failure in rpc_rawtransaction.py
< gwillen>
huh, as a result of this I went down a rabbit hole and learned about the undocumented github pull/n/merge refs that Travis uses to test the "if it were merged right now" version of a PR
< gwillen>
it's interesting to see that they show as passing, I guess because we do manual merges, so our merge was at a slightly different time than travis's merge
< fanquake>
The merge script should always be on top of master. The travis test run would be been the changes on top of master when the PR was last modified, so ~ a week ago.
< aj>
fanquake: rpc_rawtransaction seems to work fine for me (compiled with gcc)
< fanquake>
aj is that with master + 18026 ?
< aj>
just 18026, but it's directly on top of master
< aj>
1115ba693b6f6e216cd8417aa499fd018a7c016e to be exact
< fanquake>
aj hmm. Passed two times in a row now. So maybe just a random failure.
< meshcollider>
Sorry, my fault completely. I didn't run the tests after the merge script, i merged it in manually when i tested, so it was not on an entirely up-to-date master
< meshcollider>
thanks for catching and fixing aj
< fanquake>
meshcollider going to have to buy him a pint hah
< meshcollider>
in SF, sure ;)
< aj>
fanquake: do you have the logs for the failure? i think it might have the partial txs it was trying to combine logged at DEBUG level
< aj>
fanquake: i don't get it, the lines matching 'complete...False' should be the output of signrawtransaction, but they appear not to specify any vins just a weird vout, when they should be matching rawTx2 which unfortunately isn't printed in the log
< aj>
oh, hmm, now one of them looks sensible to me
< bitcoin-git>
bitcoin/master faf4081 MarcoFalke: test: Make msg_tx a witness tx
< bitcoin-git>
bitcoin/master fa6b57b MarcoFalke: test: Fix whitespace in p2p_permissions.py
< bitcoin-git>
bitcoin/master aaaae4d MarcoFalke: test: Add p2p test for forcerelay permission
< bitcoin-git>
[bitcoin] laanwj merged pull request #17984: test: Add p2p test for forcerelay permission (master...2001-qaTxForceRelay) https://github.com/bitcoin/bitcoin/pull/17984
< wumpus>
kallewoof: I'll take a look at it
< kallewoof>
wumpus: Appreciate it :) (And I meant PR, not BIP)
< bitcoin-git>
[bitcoin] practicalswift opened pull request #18029: tests: Add fuzzing harness for AS-mapping (asmap) (master...fuzzers-asmap) https://github.com/bitcoin/bitcoin/pull/18029
< gwillen>
so I got an email about my PR, saying that "bitcoin-core-ci" failed -- is this an experimental alternative to Travis and Appveyor? Is it known-broken? It appears to have failed due to reasons unrelated to the PR.
< bitcoin-git>
[bitcoin] Sjors opened pull request #18030: doc: Coin::IsSpent() can also mean never existed (master...2020/01/doc_is_spent) https://github.com/bitcoin/bitcoin/pull/18030
< wumpus>
we've managed to merge a few things this week! the first PR for the asmap-based clustering of peers went in, and step 3 of sipa's serialization improvements
< wumpus>
that leaves 7 blockers, 1 bugfix and 6 items chasing concept ACKs
< jeremyrubin>
#17925 I think is more or less RTM, and there's a lot of work waiting on it. Not sure it needs to go in high prio since things seem to be moving that way.
< jeremyrubin>
The general goal is to get these changes reviewed and merged in a more orderly fashion
< jeremyrubin>
And prevent things from suffering the tradeoffs of small PRs and big PRs by more clearly presenting what the projects are
< kanzure>
hi.
< wumpus>
thanks for the explanation, I found "mempool improvements" is a bit vague for a project name as it doesn't really aim at a specific goal
< jeremyrubin>
One of the first projects is to refactor almost every traversal algorithm in the mempool to use Epochs
< jeremyrubin>
This should be an enormous performance improvement, but the goal is not to improve performance nescessarily, but rather to permit larger descendants limits
< wumpus>
awesome!
< jeremyrubin>
Increasing the descendants limits (or making some new policys) is going to be neccessary to make Lightning-y stuff work better (and CTV)
< jeremyrubin>
Because currently there are issues with "pinning" caused by descendants limits
< wumpus>
you also might want to write this up somewhere else than IRC so it doesn't get lost :)
< hebasto>
what is an estimation of future descendants limits?
< wumpus>
maybe the project description
< jeremyrubin>
None at present
< jeremyrubin>
wumpus: will do
< jeremyrubin>
In conjunction with/after the epoch mempool improvements, it then becomes possible to make a lot of the mempool algorithms have no "short lived" allocations
< fjahr>
jeremyrubin: cool that you are coordinating this but is there an endgame to this or is the plan to keep this open indefinitely? Just curious...
< jeremyrubin>
We allocate a ton of memory in mempool traversal, these allocations can basically go to zero in a lot of places by having some preserved scratch space
< jeremyrubin>
fjahr: It's sort of indefinite, but I would like to get all the changes through to the point that we solve these higher order goals, but then maybe we won't need more stuff in the mempool
< jeremyrubin>
* major changes == stuff
< jeremyrubin>
Also as a part of this work we've lumped in amiti's rebroadcasting and sdaftuar's packagerelay work
< jeremyrubin>
Both of these should greatly help with making the mempool more rational, and better traversal algorithms help package relay be less DoS-able
< wumpus>
well, closing projects is a whole different issue, we still have "libconsensus" and "P2P refactor" open despite not having had work in progress for quite some time
< jeremyrubin>
There are also, looking forwards, some more changes inspecting the interface between mempool mining and validation
< jeremyrubin>
There's a general notion of figuring out a "streaming createnewblock" algorithm that always immediately returns the best block but does constant gradient descent to find better ones
< bitcoin-git>
[bitcoin] jonatack closed pull request #17535: test: add block height test to listsinceblock.py (master...rpc-wallet-blockheight-followups) https://github.com/bitcoin/bitcoin/pull/17535
< jeremyrubin>
This in general can allow us to have more expensive to traverse mempool graphs (so we can not have restrictions that create pinning)
< jeremyrubin>
But when mining we can still quickly return "good" blocks
< jeremyrubin>
There are a few other projects being considered, such as Child*ren* pay for parent, Cousin-RBF (instead of Conflict-RBF), and some other optimizations
< jeremyrubin>
As a bedrock to some of this, we need to have much better instrumentation and testing of the mempool
< jeremyrubin>
There are a lot of edge cases currently not tested anywhere
< jeremyrubin>
We should test these!
< jeremyrubin>
We also don't have a good asymptotic framework for microbenches\
< jeremyrubin>
We should do that (see nanobench)
< jeremyrubin>
So to summarize a bit (and then maybe more questions):
< jeremyrubin>
1) There's a lot of exciting work being shaped out & plotted for the mempool
< jeremyrubin>
2) If you're excited about the design space/working on this, please let me know
< jeremyrubin>
3) Key to making this happen is clear communication, but also keeping review motivated for sometimes small PRs as a part of a bigger picture
< jeremyrubin>
4) keeping non-functional PRs (or other mempool work) limited in scope/in consultation with the project, to keep prioritization in focus & not cause a crapload of rebase hell as these projects will have potentially a lot of un-PR'd code
< jeremyrubin>
If you disagree with 4 it's fine, ultimately up to the contributors & maintainers, but I'm trying to chart a course that's going to keep these projects moving forward
< jeremyrubin>
We could maybe open a sub-channel for this stuff on IRC if people want Yet Another IRC Channel
< jeremyrubin>
Any questions?
< jonasschnelli>
as for testing, I think kallewoof has some recordings and a test framework (AFAIK)
< wumpus>
it's not like a lot of discussion is happening in this channel lately, IMO it's fine (and preferable) to do so here
< wumpus>
you can always decide to create another channel if it reall yends up monopolizing the channel (e.g. I guess that's why #bitcoin-builds is separate)
< wumpus>
it's up to you of course
< fanquake>
What do you mean by non-functional PRs in 4)? Is there a specific set of things that you'd like to have done for 0.20.0?
< jeremyrubin>
Things which should have no user observable change
< MarcoFalke>
I think for 0.20.0 we should focus on amiti's rebroadcast stuff
< jeremyrubin>
E.g., moveonly
< jeremyrubin>
renaming variables
< wumpus>
MarcoFalke: agree, it wold be really nice to get the mempool privacy in
< amiti>
:D
< jeremyrubin>
I agree modulo concerns raised by some about needing more understanding of it.
< sipa>
jeremyrubin: are there any PRs being merged that are just moveonly/variable rename stuff? that shouldn't be the case
< jeremyrubin>
sipa: there are
< MarcoFalke>
sipa: I removed a bunch of ::mempool
< MarcoFalke>
I think jeremyrubin is referring to that
< jeremyrubin>
there are other ones too that have a lot of acks and stuff not yet merged
< jeremyrubin>
I guess it would be nice to have sort of the expectation that stuff isn't getting merged for now
< wumpus>
please be more specific
< jeremyrubin>
sure, didn't want to pick on anyone's PR but will give an example
< wumpus>
also: feel free to comment these kind of things *in* the PRs
< wumpus>
if something has a lot of ACKs and no one is pseaking against it, it tends to get merged
< jeremyrubin>
jnewbery: I agree, but the point is that if we're maintaining a few different complex projects on mempool stuff, and we're trying to slice it into many PRs to make review easier, it just becomes a headache
< MarcoFalke>
Yeah, maybe we could postpone 17786 for until after the functional/user-facing mempool changes got in?
< sipa>
yeah, if there are obvious things that interfere, it's best to discuss in the PR itself; maybe one of the authors has no problem rebasing on top of the other for example
< sipa>
or discuss what can wait
< wumpus>
yes, what sipa says
< sipa>
but it's hard to discuss a blanket "please don't do things that interfere with my work"
< MarcoFalke>
I think we can have a rough sketch in what order things should be merged. It can always be amended as needed
< wumpus>
right
< jeremyrubin>
Yeah it's fine, this is part of the goal of the mempool project
< sipa>
ok
< provoostenator>
It's useful to make Draft PR's so that Drahtbot can warn others about overlap.
< jeremyrubin>
to triage the priority of these things
< wumpus>
yes, makes sense
< wumpus>
but please: comment these things on github on the issue too, otherwise it'll likely be forgotten at some point
< jeremyrubin>
fanquake: I'd like to get Amiti's stuff improved. I'd really like to get package relay shipped. And I think it's possible to do enough work on the Epoch Mempool to bump the descendants limit by 2x.
< MarcoFalke>
agree with wumpus
< jeremyrubin>
amiti and I are discussing how to chop up her work to get the PR complexity down, I think the worry is that the earlier PRs don't do anything "useful", except to make the later work easier (things like new testing harnesses)
< fanquake>
jeremyrubin: ok. Re the non-PR'd/WIP changes, are they linked to from the project as well?
< jeremyrubin>
fanquake: yes, in general
< jeremyrubin>
If there's code
< jeremyrubin>
Things that are still design stage not really
< fanquake>
Given the amount of basic fuzzing harnesses we're adding at the moment, I don't think a mempool test harness would be rejected
< wumpus>
more fuzzing harnesses is good
< jeremyrubin>
I think it's possible it would be bikeshedded though, which I'd want to avoid
< MarcoFalke>
jeremyrubin: if you refer to style feedback with "bikeshedding", keep in mind that a valid response to style feedback is a simple "no, I like the current style because I don't see the benefit of switching to something else"
< wumpus>
which reminds me, we need to merge the examples in the -qa repository
< MarcoFalke>
wumpus: Already did
< fanquake>
I think that has been done
< wumpus>
MarcoFalke: thanks
< jeremyrubin>
MarcoFalke: in this case more of a "we could also Mock out XXXX in this harness too!"
< jeremyrubin>
Whereas for the testing harness we need to just introduce YYYY
< MarcoFalke>
In general I am not a fan of mocking all the stuff
< MarcoFalke>
If it can be tested reasonably without mocking, it should be done without
< jeremyrubin>
MarcoFalke: fair, which is sort of what I'm pointing to?
< MarcoFalke>
I guees, yes
< jeremyrubin>
I.e., do we need to mock this? What if we do x y z instead?
< jeremyrubin>
But amiti has already done a lot of hard work in making a mock framework and writing tests against that framework
< sipa>
this is a very abstract discussion
< jeremyrubin>
oops I went concrete
< amiti>
yeah... it seems like we're discussing my proposal to mock the scheduler, which marco and jeremy are both aware of... I think this convo makes more sense for when I open an actual PR
< jeremyrubin>
sgtm
< wumpus>
we still have two topocs to go and about 15 minutes
< sipa>
and the question of mocking vs testing things otherwise seems very much a case by case discussion
< jeremyrubin>
yeah
< MarcoFalke>
Yes, it is case-by-case
< MarcoFalke>
ok, other topics?
< jeremyrubin>
we can move on unless there's a new question unanswered, I'll also be online after meeting
< kanzure>
right, so, i'd like suggestions so i can write a document giving an overview of what people would like to hear
< kanzure>
for the physical IRL meeting
< kanzure>
these are just suggestions and not actually a schedule or anything draconian like that.
< jeremyrubin>
Where should we send them? Is there a google form or just email you?
< provoostenator>
I think jnewbery has been collecting topics too.
< kanzure>
just send em to me.
< jeremyrubin>
kanzure: I wouldn't *mind* a schedule ;)
< kanzure>
right or him
< fanquake>
kanzure will this be public somewhere before the meetup?
< kanzure>
well in the past they have been semi-public (link circulated) but not actually public
< kanzure>
i think that's up to the group really. do you prefer public or private topic suggestions?
< fanquake>
Sure, semi-public
< jeremyrubin>
public; I can't remember which ones I already submitted
< jnewbery>
yeah, I've also sent out a survey to some people. Thanks to everyone who responded (which is most!)
< kanzure>
anyway, for topics, it's not just what you have been working on, but also things that you think the group would benefit from hearing from someone else
< provoostenator>
jeremyrubin: downside of doing open source work where everything is logged is that you tend to develop a write-only memory :-)
< wumpus>
hehe
< * jeremyrubin>
searches for my memex
< kanzure>
that's all i have.
< wumpus>
#topic nanobench (jeremyrubin)
< jeremyrubin>
So I'm not the maintainer of nanobench
< jeremyrubin>
but I really like it
< jeremyrubin>
I think if we can do a cursory check it's not actually malware
< jeremyrubin>
we should just merge it
< wumpus>
general question: why do people want to switch to another benchmarking framework?
< wumpus>
what's wrong with the current one?
< jeremyrubin>
a few things:
< wumpus>
a while ago there was a PR to switch it to boost::test, now yet another dependency
< jeremyrubin>
1) It's slow
< wumpus>
is there anything they do that we cannot do?
< MarcoFalke>
Yeah, I think we need to take a closer look to see where they differ and what they improve. To make sure there are no regressions
< jeremyrubin>
2) there's no support for testing asymptotics
< wumpus>
what makes it so slow? it's very simple
< MarcoFalke>
For example, a lot of tools rely on the output format of the current bench framework
< wumpus>
it should hardly have any overhead
< jeremyrubin>
I think it runs too many trials
< wumpus>
then reduce that?
< jonasschnelli>
which could be changed,... right?
< jeremyrubin>
I beleive nanobench autodetects variance or something
< jeremyrubin>
martinus also claims it's more accurate -- less variance than with old benching framework
< wumpus>
so does it use a different clock?
< jeremyrubin>
It also measures more things
< wumpus>
how can accuracy differ?
< wumpus>
or does it do CPU/OS-specific cache flushing?
< sipa>
i think the current code we have is kinda crap; it started off being kinda general and automatically measuring things, and when it was shown that it introduces inaccuracies, it was changed to needing iterations counts in the code itself
< sipa>
wumpus: i think it's mostly due to some tests running far longer than necessary, resulting in getting OS interrupts etc inside of them
< jeremyrubin>
w.r.t. output and tooling, nanobench also outputs new information (e.g., instructions, cycles, branches, ips, branch misses)
< jeremyrubin>
so we'd fundamentally need some new tools.
< jnewbery>
is there anything that needs discussing here that isn't covered in the PR? I think the only action is to review that if you're interested, no? (#18011)
< wumpus>
we used to measure cycles, this was removed at some point
< sipa>
yeah let's discuss in the PR
< jeremyrubin>
Sounds good -- my point in making it a topic was that it's relatively low risk to adopt as nothing really relies heavily on the benching
< wumpus>
sounds good to me, I was just curious why everyone wants to replace the benchmark framework (with different things)
< jeremyrubin>
and being able to write asymptotic benches is going to be a big help
< wumpus>
but if the current one is crap that's clear :)
< jeremyrubin>
Because we need that for the mempool work
< jeremyrubin>
And I tried to introduce it in a since-closed PR, but it seemed we needed a more thought out approach
< jeremyrubin>
and nanobench has that
< jeremyrubin>
fin
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jan 30 19:57:07 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< kanzure>
send me a message if you want a link to the topic collection file i am preparing.
< wumpus>
at least nanobench is only one cpp+header file (right?), that's nice
< sipa>
yeah
< jeremyrubin>
yeah
< jeremyrubin>
I also like that we have an active maintainer for it
< jeremyrubin>
e.g., it's written by martinus
< wumpus>
would dread to depend on the google monstrosity
< wumpus>
yes!
< sipa>
wumpus: our current bench framework was based on google's :)
< jeremyrubin>
One interesting thing is if it's hard to backport nanobench
< jeremyrubin>
Then collect nanobenches from prior releases
< wumpus>
sipa: true, but I meant subtreeing it
< jeremyrubin>
But that sounds annoying
< jeremyrubin>
wumpus: I suggested that martinus make it a subtree; which it sounds like he could use a bit of guidance on how best to do that
< wumpus>
jeremyrubin: backporting tests and benchmarks is ok with me if you want to, it's outside the things we normally backport (bugfixes) but also has zero risk for users
< jonasschnelli>
I guess there is no need to backport it for a release
< jonasschnelli>
A local backport should be enough to collect and compare
< jeremyrubin>
jonasschnelli: yeah that's what I meant
< wumpus>
the thing with subtrees is that we end up with all kinds of unnecessary stuff in our tree
< wumpus>
esp. if it is only a cpp/h pair, we don't subtree tinyformat for a similar reason
< jeremyrubin>
Maybe martinus can make a new git repo which is just the header
< jonasschnelli>
just copy it in?
< jeremyrubin>
and then make the nanobench repo with build stuff have a subtree of that
< wumpus>
jonasschnelli: this is what he did right now and that's ok imo
< jeremyrubin>
and then subtree the repo that's just the header
< jeremyrubin>
wumpus: yeah if you're good with copy'd in then let's go that way.
< jeremyrubin>
I think we just want to avoid diverging from upstream
< jonasschnelli>
a subtree needs also maintenance and pulling
< sipa>
unrelatedly, should we work on moving all subtrees to a single location? (e.g. src/subtrees/...) ?
< jeremyrubin>
diverge meaning adding special cased stuff into nanobench rather than having a general feature that gets upstreamed/reviewed by other nanobench users
< wumpus>
sipa: I don't know
< fanquake>
I think we've got an issue open for the sun tree consolidation
< fanquake>
*subtree
< wumpus>
yes, looking for it
< jeremyrubin>
wumpus: maybe just tell martinus that copied in is fine for now, and we can consider subtreeing later?
< wumpus>
sipa: it would definitely make some things easier, but it'd also be quite a bit of disruption
< jeremyrubin>
I'll leave a comment on the PR.
< wumpus>
anyone oppposed to cleaning up the projects list a bit and closing "P2P refactor" and "libconsensus"? the former is finished, AFAIK, the latter has not had any active work for more than a year
< wumpus>
they can always be reopened if needed
< sipa>
sure
< sipa>
i mean, not opposed
< sdaftuar>
sipa: any further thoughts on #17951? would like to know if that is ready for merge, i have a wtxid-based-inv proposal ready to follow that up
< gribble>
https://github.com/bitcoin/bitcoin/issues/17951 | Use rolling bloom filter of recent block txs for AlreadyHave() check by sdaftuar . Pull Request #17951 . bitcoin/bitcoin . GitHub
< sipa>
sdaftuar: yeah, let's improve the wiping later
< sipa>
will will review after lunch
< sdaftuar>
thanks!
< jeremyrubin>
sdaftuar: worth noting that your parameter selection puts you above the max filter size I think
< bitcoin-git>
[bitcoin] achow101 opened pull request #18032: Output a descriptor in createmultisig and addmultisigaddress (master...createms-descriptor) https://github.com/bitcoin/bitcoin/pull/18032
< elichai2>
Saw the thing MarcoFalke said about tools relying on the output of the benchmark utility, should I drop the commit that tries to make it more readable then? https://github.com/bitcoin/bitcoin/pull/18014
< jeremyrubin>
probably? also it's likely that we'll soon replace bench, so might be wasted effort anyways? is the outup still intelligible?