19:00:20 <wumpus> #startmeeting
19:00:20 <lightningbot> Meeting started Thu Apr 30 19:00:20 2020 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:20 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:34 <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
19:00:36 <wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55
19:00:38 <jonatack> hi
19:00:46 <MarcoFalke> hi
19:01:08 <wumpus> no proposed topics in http://gnusha.org/bitcoin-core-dev/proposedmeetingtopics.txt
19:01:12 <meshcollider> hi
19:01:12 <wumpus> any last minute suggestions?
19:01:22 <jnewbery> hi
19:01:50 <ariard> hi
19:01:54 <hebasto> hi
19:02:12 <wumpus> #topic High priority for review
19:02:27 <MarcoFalke> Can I add #18699 ?
19:02:30 <gribble> https://github.com/bitcoin/bitcoin/issues/18699 | wallet: Avoid translating RPC errors by MarcoFalke · Pull Request #18699 · bitcoin/bitcoin · GitHub
19:02:36 <kanzure> hi
19:02:41 <wumpus> https://github.com/bitcoin/bitcoin/projects/8  -> 5 blockers, 1 bug fix, 5 chasing concept ACK
19:02:47 <luke-jr> #18818 needs a 0.20 tag
19:02:48 <gribble> https://github.com/bitcoin/bitcoin/issues/18818 | Fix release tarball generated by gitian by luke-jr · Pull Request #18818 · bitcoin/bitcoin · GitHub
19:02:49 <wumpus> MarcoFalke: sure
19:03:16 <amiti> hi
19:03:46 <luke-jr> at *least* the first commit there is needed - but IMO both should be fixed
19:03:50 <achow101> hi
19:04:19 <MarcoFalke> luke-jr: Doesn't that need to re-add the "version hack"?
19:04:32 <achow101> add #16946 to hi prio pls
19:04:35 <gribble> https://github.com/bitcoin/bitcoin/issues/16946 | wallet: include a checksum of encrypted private keys by achow101 · Pull Request #16946 · bitcoin/bitcoin · GitHub
19:04:53 <luke-jr> MarcoFalke: I don't know why the version hack was removed, but if it works in master, it should still work with these fixes I think
19:05:07 <sipa> what is the version hack?
19:05:16 <wumpus> achow101: ok
19:05:31 <luke-jr> sipa: for several releases, we've hand-made build.h in gitian because otherwise it would get some commit hash as the version number
19:05:38 <ariard> dunno if it's necessary to add to high prio, it's already hot, but just pushed #16426 last version
19:05:42 <gribble> https://github.com/bitcoin/bitcoin/issues/16426 | Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard · Pull Request #16426 · bitcoin/bitcoin · GitHub
19:05:43 <wumpus> fwiw I'm fine with the current tarball which is just a dumb dump of the complete git repository
19:05:44 <luke-jr> or -dirty added, I forget the details :/
19:05:58 <luke-jr> wumpus: without a leading directory..
19:06:00 <MarcoFalke> Looks like sipa has two pulls in high prio :eyes: NoT AllOweD!!!111
19:06:11 <sipa> oh?
19:06:23 <wumpus> MarcoFalke: oh no!!!
19:06:25 <sipa> oh yes
19:06:30 <MarcoFalke> nooooooo
19:06:36 <sipa> pick one
19:07:11 <luke-jr> wumpus: ie, if you extract it, you get the contents dumped into your current directory
19:07:40 <MarcoFalke> luke-jr: The previous releases had the prefix? If so, it makes sense to add it back
19:07:44 <wumpus> luke-jr: that's kind of an unexpected change from before
19:07:48 <sipa> yeah, agree
19:07:59 <sipa> no opinion on the pre/post configure aspect
19:08:03 <wumpus> luke-jr: is it possible to add the prefix without going through the whole extract and repack dance again?
19:08:16 <luke-jr> wumpus: yes, the first commit of my PR does that
19:08:21 <wumpus> I was kind of happy to get rid of that
19:08:23 <wumpus> okay!
19:08:26 <wumpus> will take a look then
19:08:26 <jonasschnelli> hi
19:09:03 <luke-jr> I was also tempted to use tar --update to merely append to the git-archive tarball, but that seems dangerous considering timestamps
19:09:18 <wumpus> okay everything should be tagged / added to high prio
19:09:35 <MarcoFalke> luke-jr: Maybe split out the first commit into a pull, so that gitian builds can be performed on it more easily?
19:10:05 <hebasto> Agree ^
19:10:10 <luke-jr> MarcoFalke: well, like I said, IMO both should be fixed - so unless there's a strong objection to distcleaning, I'd rather focus on that for now?
19:10:54 <luke-jr> I don't see the motivation in deviating so significantly from best practices in that regard..
19:11:21 <wumpus> well for what it's worth there seems to be not disagreement at all on adding the prefix so that should be straightforward even to get into rc2
19:11:43 <luke-jr> is there disagreement on the rest? or just people didn't feel like doing it before?
19:11:43 <jonatack> I wonder if it makes sense to remove #16442 until it's rebased, the longer it stays in the blockers without action, the more reviewers might be tuning it out
19:11:47 <gribble> https://github.com/bitcoin/bitcoin/issues/16442 | Serve BIP 157 compact filters by jimpo · Pull Request #16442 · bitcoin/bitcoin · GitHub
19:12:37 <wumpus> the other change requires a unpack and configure and re-pack, I don't really like that, I was happy to just use git to create the archive
19:12:40 <MarcoFalke> jonatack: Yeah, generally we remove stuff that needs rebase
19:12:50 <wumpus> jonatack: sgtm
19:13:01 <sipa> if luke's PR is all it takes to get us back to post-configure (without other hacks) it seems reasonable to take it
19:13:27 <sipa> my impression was that we got rid of the post-configure thing because it was hard to maintain
19:13:34 <sipa> would that still be the case with this approach?
19:14:25 <MarcoFalke> Yes, the issue was that files were missing from the tar.gz
19:14:58 <fjahr> hi
19:15:03 <luke-jr> That issue remains fixed with this approach
19:15:35 <wumpus> it at the least makes the tarball build process more complex again, which potentially introduces bugs and inconsistencies again
19:16:37 <wumpus> in any case if the PR gets enough ACKS it can go in
19:17:25 <hebasto> split in two prs is also a way
19:17:27 <wumpus> at the least I hope this fixes it and we're not going to have lots of back and forth on this
19:17:36 <wumpus> which is the feeling I get if people talk about version hacks
19:17:49 <cfields> I
19:18:01 <cfields> I'm still having trouble understanding why this was all changed.
19:18:36 <wumpus> because we wanted all files in the tarball
19:19:09 <wumpus> and a safe way to do that is 'git archive', it doesn't accidentally include any junk files nor does it exclude anything
19:19:18 <luke-jr> cfields: the previous problem was that `make dist` missed a lot of files
19:19:27 <cfields> sure, understood.
19:19:59 <cfields> but now we're talking about bootstrapping and appending anyway. but we're appending anyway. We could've just bootstrapped as before and appended the git archive... the reverse of this I suppose.
19:20:24 <wumpus> doing the same with make dist would involve including everything in the makefile, either through wildcards or worse, through enumeration, something impossible to keep up to date
19:20:29 <cfields> as long as it works, I suppose.
19:20:31 <wumpus> yes, that' pretty much what luke-jr  does in his PR
19:21:20 <wumpus> let's just review that I guess
19:21:33 <cfields> sgtm
19:22:32 <ariard> luke-jr: seeing your comment https://github.com/bitcoin/bitcoin/pull/18797#issuecomment-622008886
19:22:58 <ariard> I agree it's not ideal to export standardness among libconsensus and maybe we should a clear distinct libstandard
19:23:08 <wumpus> is that a topic?
19:23:20 * MarcoFalke lets do libbitcoincore
19:23:23 <wumpus> #topic Standardness in libconsensus
19:23:33 <ariard> but people on higher layers definetely need to be sure that their ttxn are going to propage on the currently deployed network
19:23:43 <ariard> wumpus: I should have submitted before sorry
19:23:51 <wumpus> ariard: np
19:23:54 <luke-jr> ariard: maybe stuff like c-lightning should just call RPC?
19:24:10 <ariard> luke-jr: you want to do this in your test framework
19:24:18 <ariard> and you may not have a full-node to test
19:24:21 <luke-jr> test frameworks can run a bitcoind
19:24:23 <wumpus> MarcoFalke: the problem is defining a sane interface
19:24:37 <wumpus> has always been; the idea to extend libconensus has been there for very long
19:24:42 <ariard> luke-jr: it's make integration harder
19:24:55 <MarcoFalke> ariard: Doesn't lightning depend on Bitcoin Core or a different full node impl anyway?
19:25:17 <luke-jr> ariard: well, I don't see a sane way to address user choice of policies..
19:25:25 <ariard> MarcoFalke: actually no, you can be connected to an electrum server but still want to verify a single-sricptpubkey
19:25:26 <wumpus> yes, c-lightning depends on bitcoin core (at run time, not at build time)
19:25:26 <luke-jr> another sane way*
19:25:33 <wumpus> oh did that change?
19:25:39 <ariard> beyond c-lightning or any LN implementation
19:25:42 <cfields> at the very least I think we'd want it broken out into a new function that's clearly non-consensus-trustworthy.
19:26:15 <ariard> cfields: yes, let's not confuse what is standard and consensus
19:26:30 <luke-jr> there's other utility RPCs that would make sense to export somewhere too
19:26:32 <sipa> i see no problem exposing a Bitcoin Core library that implements the same policies as Bitcoin Core; it's just a more convenient interface to do something you can already do with running a node
19:26:41 <luke-jr> libbitcoincore isn't the best name, but I can't think of a better one XD
19:26:52 <MarcoFalke> lol, I was trolling
19:26:55 <wumpus> I don't see a problem either, just make it a different function from consensus validation
19:26:56 <sipa> it seems convenient to add that to bitcoinconsensus, but i agree it's confusing the name of the lib
19:27:10 <ariard> okay we should rename bitcoinconsenus to something different?
19:27:12 <MarcoFalke> ariard: is elecrum server not a full node?
19:27:22 <luke-jr> I suppose we could put it in the same lib, but add a second .pc file in case we split it later..
19:27:23 <wumpus> no, don't rename bitcoinconsensus
19:27:34 <luke-jr> otoh, do callers of these functions need consensus code at all?
19:27:38 <ariard> MarcoFalke: I meaned a electrum client serving as a verification backend to a LN node
19:27:42 <ariard> like a mobile
19:28:01 <ariard> you still want to verify scriptpubkey if you can even if you can't verify full set of consensus/standard ruels
19:28:15 * luke-jr saves the rant against people not using their own full nodes for another time
19:28:34 <wumpus> how do the other mobile LN wallets do that? they definitely don't all require a full node
19:28:41 <MarcoFalke> Why would you not use the electrum server and instead hop through the electrum client?
19:28:54 <ariard> luke-jr: my mobile connected to my full-node may know network outage and still need to update a channel state
19:28:54 <luke-jr> doesn't Lightning use very specific templates anyway?
19:29:09 <luke-jr> so long as the standard Lightning scripts are being used, they should all be fine, no?
19:29:21 <ariard> wumpus: you may use BIP157 or any other light client protocols, even a custom one
19:29:46 <ariard> luke-jr: we use specific templates but witness must enforce MINIMALIF among others
19:29:47 <sipa> i think we're straying from the topic now
19:30:06 <wumpus> in any case, I'm not against adding a standard check function, just call it differently, I don't think it matters it's in libconsensus *if* you document it and name it in a clear way
19:30:13 <ariard> right, we do agree exporting a standard *opaque* flag? it's just a way to do it
19:30:21 <sipa> ariard: if you use the standard template, they are minimalif automatically?
19:30:35 <wumpus> no, I don't want to do it with a flag, just add a function
19:30:48 <wumpus> don't touch the consensus function for non-consensus checks
19:30:50 <ariard> sipa: don't follow there what you mean by standard template?
19:31:07 <sipa> ariard: lightning uses specific scripts, no?
19:31:11 <sipa> bolt 03 or something
19:31:16 <ariard> okay so add a verify_standard function but get it in bitcoinconsensus library?
19:31:29 <wumpus> yes
19:31:37 <luke-jr> ariard: add a new .pc file too IMO
19:31:51 <wumpus> doesn't seem worth it to add a new library for, but should be a separate function imo
19:32:00 <wumpus> luke-jr: nah, there will be so much overlap between both
19:32:04 <ariard> luke-jr: it may makes intgration harder, dunno who is using this right now apart of rust-bitocin ecosystem
19:32:11 <luke-jr> wumpus: it's like 4 lines?
19:32:17 <wumpus> either the two libraries have to dpeend on each other
19:32:19 <wumpus> or share code
19:32:32 <luke-jr> wumpus: just a new .pc, not a new lib
19:32:57 <sipa> luke-jr: what is the difference?
19:33:12 <ariard> I think important point is document well in shared-libraries.md that it's standardness
19:33:36 <luke-jr> sipa: .pc is like 4 lines of text that says which linker/compiler flags to use
19:33:41 <luke-jr> so for now it would just -lconsensus
19:33:56 <luke-jr> if down the road it gets big enough to split up, we can change to -lsomethingelse
19:33:57 <ariard> and you may have to call verify_standard and then verify consensus only to know invalidity cause of your transaction
19:34:01 <wumpus> yes, the important point is to name it clearly
19:34:02 <luke-jr> and other stuff using it will just work
19:34:03 <wumpus> and document it clearly
19:34:14 <wumpus> no need to split it out into yet another library or mess around with .pc files IMO
19:34:31 <sipa> ariard: btw, bitcoin core internally also calls script verification twice to determine cause of failure (once with and once without standardness rules enabled)
19:34:52 <sipa> at least in some cases
19:35:05 <ariard> yes exactly in mempool acceptance, that what I had in mind
19:35:19 <sipa> that said, i'm still a bit confused why you need this
19:35:23 <cfields> how about just make a copy of bitcoinconsensus.cpp, make the policy changes to the new one, and maintain them in parallel?
19:35:30 <cfields> That's how we always say we'll handle stuff like this, anyway :)
19:35:37 <sipa> heh
19:36:02 <luke-jr> I would assume it's a new .cpp file anyway
19:36:05 <ariard> sipa: you want to verify your witness builder and you may have a lot of different cases in LN
19:36:33 <sipa> ariard: if you follow BOLT 3, that shouldn't be needed, right?
19:37:07 <sipa> (i understand the use for testing, but the point of having a lightning standard seems to be that it removes the need for doing this in production)
19:38:24 <MarcoFalke> regtest should have all standardness flags like mainnet, so testing against regtest during development should be sufficient (haven't checked)
19:38:39 <MarcoFalke> testmempoolaccept in regtest
19:38:47 <luke-jr> MarcoFalke: +1
19:39:31 <MarcoFalke> #15891
19:39:35 <gribble> https://github.com/bitcoin/bitcoin/issues/15891 | test: Require standard txs in regtest by default by MarcoFalke · Pull Request #15891 · bitcoin/bitcoin · GitHub
19:40:15 <ariard> you may also want to check in production, witness may contain input from different parties
19:40:30 <cfields> If we're doing a new policy function, it would be nice to return a bitfield of failed (non-fatal) checks rather than a true/false. That may require too much interpretor refactor to be safe, though.
19:40:40 <sipa> cfields: hard nack
19:40:50 <luke-jr> ariard: you shouldn't care about what the inputs are..?
19:40:52 <cfields> lol, that was quick.
19:41:15 <sipa> cfields: yeah, sorry - i feel very strongly about the fact that we absolutely should not expose all nitty details of how we implement standardness
19:41:52 <sipa> over time certain flags might be merged together or so
19:41:59 <sipa> or split up depending on conditions
19:42:13 <sipa> or things may get tested in different order
19:42:21 <sipa> so that the first failure reported changes
19:42:35 <luke-jr> a bitfield would require testing all even after one fails ;P
19:42:38 <wumpus> yes, agree with that, the problem with these kind of interfaces is that they're hard to update
19:42:56 <cfields> ok, fair enough.
19:43:07 <sipa> if we expose a way to test policy, it is a "test things against current bitcoin core policy", not a "learn about all the things that may be wrong"
19:43:09 <luke-jr> policy is inherently volatile, even with the same version
19:43:22 <luke-jr> there is no "bitcoin core policy", there is "this node's policy" :/
19:43:46 <ariard> yeah we shouldn't commit to policy for sure, but you're making expactations about network mempools
19:43:59 <luke-jr> ariard: which is something we don't have code for at all right now
19:44:00 <ariard> to be sure your time-sensitive tx is going to reach some miners pool
19:44:24 <ariard> luke-jr: yes that's an issue, have you followed thread about RBF pinning on the ml
19:44:31 <luke-jr> I haven't
19:45:02 <ariard> tl;dr: by pinning transaction in the mempool due to loosely understood RBF rules by LN people you may delay a time-senstivie tx
19:45:11 <ariard> and therefore steal people funds
19:45:16 <wumpus> which makes me doubt this a bit in the first place, this function accepting your transaction only means that one version of bitcoin core accepted it at one point, it doesn't tell you about the current network
19:45:46 <sipa> and can't take even your own local mempool's state into account, if you have one
19:45:52 <wumpus> right
19:46:27 <ariard> wumpus: you should test against late core version standardness, I doesn't _guarantee_  but give you confidence it's going to broadcast?
19:46:45 <ariard> *it doesn't
19:47:09 <sipa> ariard: i'm... unconvinced
19:47:21 <luke-jr> considering we don't have code for what you really need, maybe this should just be an entirely new library separate from Core?
19:47:22 <luke-jr> perhaps even adapting on-the-fly
19:47:35 <sipa> ariard: there is no reason to assume that checking against script standardness rules makes anything actually harder for an attacker
19:47:36 <ariard> sipa: on standardness verification or documenting better mempools rules?
19:48:14 <sipa> ariard: because there may be relay/validity policies that are outside script, and equally easy to attack
19:48:45 <ariard> sipa: I agree it doesn't exclude all attacks vectors but you reduce surface
19:49:15 <sipa> ariard: my point is that this sort of approach should be used for testing (is the stuff my code produced acceptable to the network)
19:49:31 <sipa> but in production there is no way around querying an actual node for acceptability
19:50:29 <ariard> let's say an attacker can degrade acceptability of your transaction by sending you non-standard stuff
19:50:46 <sipa> anyway, this is not an argument against exposing a standardness check... just be aware that its use is limited
19:50:48 <ariard> if you can verify this, without a full-node access, you should be able to do so
19:51:22 <sipa> ariard: if an attacker can just move to an equally-cheap attack you can't detect, have you gained anything?
19:51:50 <sipa> (if you have an argument why that is substantially harder to attack non-script stuff, you would have a point)
19:51:59 <ariard> sipa: I see a difference between standardness on the script and standardness on mempool acceptance
19:52:19 <ariard> like in RBF pinning as exposed on the ml, attacker has to commit a tx which may confirm in some block
19:52:25 <ariard> and so loose a fee by trying the attack
19:52:39 <sipa> ok, so there is an extra cost to bypassing script... that's a good argument
19:53:20 <luke-jr> hmm, got d/c - apparently logs did too
19:53:27 <luke-jr> sipa: no code we have can determine that
19:53:32 <luke-jr> [19:45:27] <luke-jr> considering we don't have code for what you really need, maybe this should just be an entirely new library separate from Core?
19:53:34 <luke-jr> [19:46:33] <luke-jr> perhaps even adapting on-the-fly
19:54:00 <sipa> you're welcome to write that :)
19:55:07 <ariard> okay so extending bitcoinconsensus with a verify_standard function for now, with a clear documentation what's its limited and how it should be used?
19:55:29 <ariard> and I also propose to open an issue explaining different ways an attacker may exploit standard rules and different cost between them
19:55:45 <ariard> like for all this weired multi-party time-sensitive cases
19:56:11 <luke-jr> sipa: inability to configure it is a reason not to expose it
19:56:18 <luke-jr> defaults should only be just defaults
19:56:19 <wumpus> I don't think standard rules are supposed to ever be an attack vector
19:56:38 <luke-jr> ariard: I still think NACK on a policy function in lib
19:56:48 <ariard> wumpus: but sadly they are, go through ml post
19:57:03 <wumpus> isn't this only a problem for 0-conf?
19:57:20 <sipa> you don't even need RBF etc for that; LN very inherently relies on predictability of confirmation
19:57:32 <ariard> luke-jr: it's just a question on how to make them practical between -testmempoolaccept and verify_standard
19:57:43 <luke-jr> wumpus: Lightning is all about unconf
19:58:01 <luke-jr> ariard: testmempoolaccept is practical ☺
19:58:08 <ariard> wumpus: it's more subtle you blind your counterparty to avoid them having their alternative tx timing-out some output
19:58:33 <luke-jr> ariard: but will never tell you network-wide policies
19:58:37 <wumpus> luke-jr: I thought channels were only opened once the opening transaction confirms, same for other changes
19:58:44 <ariard> luke-jr: if you wnat to run your test framework quicly asking for a full-node running isn't practical
19:58:45 <luke-jr> wumpus: yes, I thought so too
19:58:58 <luke-jr> wumpus: I don't really understand the practical issue
19:59:00 <ariard> like I'm travelling and want to be able to test without network access
19:59:07 <luke-jr> ariard: yes it is
19:59:13 <luke-jr> regtest works fine without network
19:59:40 <ariard> luke-jr: it makes running test slower
19:59:44 <wumpus> I'm not sure you traveling without network access warrants adding a new function to libconsensus :)
19:59:57 <wumpus> I'm sure you could just hack something that works for yourself
20:00:00 <luke-jr> ariard: Bitcoin Core does testing this way; it works fine
20:00:18 <ariard> wumpus: yeah yeah right, it's more running test smoothly :)
20:00:36 <sipa> it is far less practical for an external project to write a test that needs to spin up a bitcoind
20:00:48 <sipa> than it is for us to spin out the thing we're testing and building ourselves
20:00:48 <luke-jr> sipa: why?
20:00:53 <sipa> come on
20:01:04 <luke-jr> you just use PATH instead of specifying the binary path…
20:01:07 <wumpus> it might be more practical than linking against bitcoin core at compile time though :)
20:01:12 <ariard> luke-jr: longuer dependencies chain
20:01:20 <luke-jr> ariard: ⁇?
20:01:32 <luke-jr> depending on bitcoind vs depending on libconsensus is the same chain
20:01:36 <wumpus> no, the depenency chains are the same in both cases, the only difference is compile time versus run time dep
20:01:39 <wumpus> yes
20:01:46 <ariard> luke-jr: you're asking people developping and testing higher stuff to have a full setup bitcoin env
20:01:49 <wumpus> we need to wrap up the meeting btw
20:01:54 <jonatack> fwit the latest optech provides a summary of the ml discussion at https://bitcoinops.org/en/newsletters/2020/04/29/#news
20:02:02 <jonatack> fwiw*
20:02:09 <wumpus> ariard: if regtest is enough, that's set up in a second or so
20:02:17 <ariard> right let's wrap-up we can keep conversation on PR
20:02:18 <wumpus> jonatack: thanks
20:02:32 <wumpus> #endmeeting