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