< bitcoin-git> [bitcoin] andrewtoth opened pull request #18025: doc: Add missing supported rpcs to doc/descriptors.md (master...patch-1) https://github.com/bitcoin/bitcoin/pull/18025
< bitcoin-git> [bitcoin] meshcollider pushed 14 commits to master: https://github.com/bitcoin/bitcoin/compare/638239de7502...2d6e76af2409
< bitcoin-git> bitcoin/master f5be479 Joao Barbosa: wallet: Improve CWallet:MarkDestinationsDirty
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/17156 | psbt: check that various indexes and amounts are within bounds by achow101 . Pull Request #17156 . bitcoin/bitcoin . GitHub
< 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: didn't check, hang on
< fanquake> After merging 18026.
< 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
< fanquake> aj yea I've got em. 1 sec
< fanquake> aj 12000 lines of consolidated log in https://gist.github.com/fanquake/48b9003af82f5ec5f1f44d1275eb21f0
< 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] gwillen opened pull request #18027: "PSBT Operations" dialog (master...feature-psbt-ops-dialog) https://github.com/bitcoin/bitcoin/pull/18027
< fanquake> aj is there a specific node you need a tx from?
< aj> fanquake: no, don't think so
< gwillen> fanquake: is the PR tagging done by a robot under your name, or are you just extremely fast
< aj> fanquake: seems like the first partially signed tx's hex isn't being decoded properly of all things?
< gwillen> (labelling I mean)
< fanquake> gwillen It's done by me. I'm normally around.
< fanquake> aj ok. I have the datadirs so can spin the nodes back up.
< aj> great
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/2d6e76af2409...44c2400bccbc
< bitcoin-git> bitcoin/master 1115ba6 Anthony Towns: psbt_wallet_tests: use unique_ptr for GetSigningProvider
< bitcoin-git> bitcoin/master 44c2400 fanquake: Merge #18026: psbt_wallet_tests: use unique_ptr for GetSigningProvider
< bitcoin-git> [bitcoin] fanquake merged pull request #18026: psbt_wallet_tests: use unique_ptr for GetSigningProvider (master...202001-getsigningprovider-fix) https://github.com/bitcoin/bitcoin/pull/18026
< achow101> damn silent merge conflicts
< achow101> too many wallet PRs conflict with each other. glad most of it's over now. just time to rebase everything
< fanquake> the bot is doing the rounds
< achow101> gwillen: fanquake's actually a robot :)
< gwillen> thus, all is explained
< aj> wtf
< aj> fanquake: this seems to be a bug in tx deserialisation ever since segwit has existed?
< aj> oh no that's too strong, i can't compile the segwit PR. hmm
< aj> fanquake: #18028 should be the problem, completely unrelated if so
< gribble> https://github.com/bitcoin/bitcoin/issues/18028 | Some transactions cant be decoded from hex strings . Issue #18028 . bitcoin/bitcoin . GitHub
< fanquake> aj: thanks for following up! Looks like a pretty rare failure then
< kallewoof> If people have time, energy, effort, will, and patience, please review the signet BIP? It feels like it's frozen in place! #16411
< gribble> https://github.com/bitcoin/bitcoin/issues/16411 | BIP-325: Signet support by kallewoof . Pull Request #16411 . bitcoin/bitcoin . GitHub
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/44c2400bccbc...0130abbdb7f5
< bitcoin-git> bitcoin/master 1b96a3c fanquake: tests: reset fIsBareMultisigStd after bare-multisig tests
< bitcoin-git> bitcoin/master 0130abb MarcoFalke: Merge #18018: tests: reset fIsBareMultisigStd after bare-multisig tests
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #18018: tests: reset fIsBareMultisigStd after bare-multisig tests (master...fix_p2sh_tests_failure) https://github.com/bitcoin/bitcoin/pull/18018
< bitcoin-git> [bitcoin] MarcoFalke pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/0130abbdb7f5...7fcaa8291c6e
< bitcoin-git> bitcoin/master 6ef0491 practicalswift: tests: Update FuzzedDataProvider.h from upstream (LLVM)
< bitcoin-git> bitcoin/master ccc3c76 practicalswift: tests: Add fuzzer strprintf to FUZZERS_MISSING_CORPORA (temporarily)
< bitcoin-git> bitcoin/master cc668d0 practicalswift: tests: Add fuzzing harness for strprintf(...)
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #18009: tests: Add fuzzing harness for strprintf(...) (master...fuzzers-strprintf) https://github.com/bitcoin/bitcoin/pull/18009
< bitcoin-git> [bitcoin] laanwj pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/7fcaa8291c6e...3b69310beb17
< 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.
< hebasto> gwillen: could be useful https://github.com/bitcoin/bitcoin/issues/17803
< hebasto> begging devs, who is interested in GUI i18n, for reviewing of #16224
< gribble> https://github.com/bitcoin/bitcoin/issues/16224 | gui: Bilingual GUI error messages by hebasto . Pull Request #16224 . bitcoin/bitcoin . GitHub
< 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
< bitcoin-git> [bitcoin] sipsorcery opened pull request #18031: Remove GitHub Actions CI workflow. (master...remove-ghaction) https://github.com/bitcoin/bitcoin/pull/18031
< wumpus> we disabled actions so you shouldn't be getting mails from that
< wumpus> not recently at least
< sipa> i get those for every PR i open
< sipa> the past days
< gwillen> this was last night
< gwillen> so they're definitely not disabled
< wumpus> where is the mail coming from?
< gwillen> From: notifications@github.com, [gwillen/bitcoin] Run failed: bitcoin-core-ci - feature-psbt-ops-dialog (b58e6f7)
< gwillen> cc: ci_activity@noreply.github.com
< gwillen> "Workflow: bitcoin-core-ci
< emilengler> gwillen: This is in your fork, I believe you can disable this
< sipa> yeah, same (but sipa/bitcoin)
< gwillen> I mean, I never intentionally enabled it
< wumpus> ohh maybe github actions is enabled for your clone
< emilengler> gwillen: The featrue is new, it was enabled for everyone
< gwillen> like, I cloned while it was enabled, and now it will stay enabled until I remove it?
< emilengler> Well it's not that new anymore
< sipsorcery> PR has been added to remove the GitHub Actions job see #17803.
< gribble> https://github.com/bitcoin/bitcoin/issues/17803 | ci: Migration from AppVeyor to GitHub Actions . Issue #17803 . bitcoin/bitcoin . GitHub
< emilengler> gwillen: Go to Settings > Notifications > GitHub actions
< sipa> ah, and having the github actions files in the bitcoin/bitcoin repo they end up in our repos too, where github automatically picks it up?
< sipa> ok, disabled
< wumpus> sipa: yes, exactly, sipsorcery's PR would remove it so it's no longer picked up
< gwillen> oh, and it runs based on the existence of the ci.yml file in the branch being PR'ed?
< gwillen> so once that file is gone in master this will stop happening
< sipsorcery> yes
< gwillen> sipsorcery: how do I disable it?
< gwillen> er sorry, sipa: ^
< gwillen> I can see a list of "actions" on my fork, but I see no way to remove or disable one (while the file exists)
< sipa> settings -> actions -> actions permissions -> "Disable actions for this repository"
< sipa> it's actually under settings
< sipa> not the top bar actions menu
< gwillen> thanks
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Jan 30 19:00:49 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< provoostenator> hi
< sipa> hi
< fjahr> hi
< emilengler> hi
< sipsorcery> hi
< promag> hi
< hebasto> hi
< gwillen> hi
< nehan_> hi
< wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball kvaciral ariard digi_james amiti fjahr
< wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55
< jonasschnelli> hi
< meshcollider> hi
< jonatack> hi
< wumpus> one pre-proposed topic in https://gist.github.com/moneyball/071d608fdae217c2a6d7c35955881d8a: topic idea collection for physical meeting (kanzure)
< elichai2> Hi
< wumpus> PSA: 0.19.1rc1 was released, please help testing and report any issues you find to the bug tracker
< jeremyrubin> Hiya!
< wumpus> also, the 0.20 feature freeze is in one and a half month (see #17432)
< gribble> https://github.com/bitcoin/bitcoin/issues/17432 | Release schedule for 0.20.0 . Issue #17432 . bitcoin/bitcoin . GitHub
< wumpus> any last minute topic proposals?
< jeremyrubin> #proposedmeetingtopic I'd love to chat about the mempool project and share trajectory
< wumpus> thanks
< wumpus> let's start with the usual then
< wumpus> #topic High priority for review
< achow101> #16528 pls
< gribble> https://github.com/bitcoin/bitcoin/issues/16528 | Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101 . Pull Request #16528 . bitcoin/bitcoin . GitHub
< 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.
< sipa> and the wallet boxes
< gribble> https://github.com/bitcoin/bitcoin/issues/17925 | Improve UpdateTransactionsFromBlock with Epochs by JeremyRubin . Pull Request #17925 . bitcoin/bitcoin . GitHub
< wumpus> sipa: yes!
< fanquake> hi
< wumpus> added #16528 and #17925
< gribble> https://github.com/bitcoin/bitcoin/issues/16528 | Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101 . Pull Request #16528 . bitcoin/bitcoin . GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/17925 | Improve UpdateTransactionsFromBlock with Epochs by JeremyRubin . Pull Request #17925 . bitcoin/bitcoin . GitHub
< jeremyrubin> thanks -- will share more details when it's my topics turn
< jeremyrubin> #proposedmeetingtopic nanobench
< wumpus> FWIW kallewoof has the idea review kind of ground to a halt on signet (#16411) and is looking for more reviewers
< gribble> https://github.com/bitcoin/bitcoin/issues/16411 | BIP-325: Signet support by kallewoof . Pull Request #16411 . bitcoin/bitcoin . GitHub
< wumpus> #topic mempool project (jeremyrubin)
< jeremyrubin> Howdy!
< jeremyrubin> So I've been working on a bunch of improvements to the Mempool with a few other contributors
< jeremyrubin> We have a project allocated here https://github.com/bitcoin/bitcoin/projects/14
< jeremyrubin> to triage work on the MemPool
< jnewbery> hi
< 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> FWIW I try to mostly focus on "high priority for review" as for what to merge lately, there's just too many PRs
< jnewbery> I think removing ::mempool in order to clarify initialization order is a sensible project
< jonasschnelli> 17786 has a single ack
< wumpus> #17786
< gribble> https://github.com/bitcoin/bitcoin/issues/17786 | refactor: Nuke policy/fees->mempool circular dependencies by hebasto . Pull Request #17786 . bitcoin/bitcoin . GitHub
< 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
< fanquake> wumpus yes
< 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
< wumpus> #topic propose physical meeting topics (kanzure)
< kanzure> just the topic collection topic.
< jeremyrubin> So not actual topics?
< 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)
< gribble> https://github.com/bitcoin/bitcoin/issues/18011 | Replace current benchmarking framework with nanobench by martinus . Pull Request #18011 . bitcoin/bitcoin . GitHub
< 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> #17413
< gribble> https://github.com/bitcoin/bitcoin/issues/17413 | Subtree exclude mess in linters and update scripts . Issue #17413 . bitcoin/bitcoin . GitHub
< wumpus> jeremyrubin: sounds good to me
< 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?
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/3b69310beb17...1d1f8bbf5711
< bitcoin-git> bitcoin/master b951b09 Larry Ruane: on startup, write config options to debug.log
< bitcoin-git> bitcoin/master 1d1f8bb MarcoFalke: Merge #16115: On bitcoind startup, write config args to debug.log
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #16115: On bitcoind startup, write config args to debug.log (master...args-to-debug-log) https://github.com/bitcoin/bitcoin/pull/16115
< elichai2> jeremyrubin: only changed spacings so the columns will be readable for humans
< fanquake> wumpus I agree
< bitcoin-git> [bitcoin] achow101 closed pull request #16528: Native Descriptor Wallets using DescriptorScriptPubKeyMan (master...wallet-of-the-glorious-future) https://github.com/bitcoin/bitcoin/pull/16528
< bitcoin-git> [bitcoin] achow101 reopened pull request #16528: Native Descriptor Wallets using DescriptorScriptPubKeyMan (master...wallet-of-the-glorious-future) https://github.com/bitcoin/bitcoin/pull/16528
< fanquake> review beg #18003
< gribble> https://github.com/bitcoin/bitcoin/issues/18003 | build: remove --large-address-aware linker flag by fanquake . Pull Request #18003 . bitcoin/bitcoin . GitHub
< jeremyrubin> sipa: I fixed my comments!
< jeremyrubin> Can anyone clarify/point to documentation on when cleanstack gets applied?
< jeremyrubin> I couldn't find anything fleshed out
< jeremyrubin> (other than,,, the code
< sipa> jeremyrubin: bip62 rule 6 is implemented as a standardness rule
< sipa> it's also required (as a consensus rule) in p2wsh
< jeremyrubin> Interesting.
< jeremyrubin> There's a kind of nice script I'm looking at for CTV which is
< jeremyrubin> <t1> ... <tn> <N> OP_ROLL OP_ROLL OP_CTV
< jeremyrubin> So I would need to follow it by a bunch of op_drops then?
< sipa> seems silly to use that post taproot
< jeremyrubin> someone asked :)
< jeremyrubin> Which is fine, no cleanstack works in v2 bare script transactions right?