< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/5948398b1866...3f1966ead6b2
< bitcoin-git> bitcoin/master b6f9e35 fanquake: test: re-enable CLI test support by using EncodeDecimal in json.dumps()
< bitcoin-git> bitcoin/master 3f1966e fanquake: Merge #17705: test: re-enable CLI test support by using EncodeDecimal in j...
< bitcoin-git> [bitcoin] fanquake merged pull request #17705: test: re-enable CLI test support by using EncodeDecimal in json.dumps() (master...test_framework_json_dumps) https://github.com/bitcoin/bitcoin/pull/17705
< bitcoin-git> [bitcoin] fanquake opened pull request #17730: depends: remove Qt networking features (master...qt_no_lib_system_proxy) https://github.com/bitcoin/bitcoin/pull/17730
< fanquake> sipsorcery: take a look at the changes in 17730. They remove the need to link against a couple dylibs on macOS. You might be able to drop similar libraries on Windows.
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #17727: WIP: Update msvc build to ignore warning and bump Qt version to 5.12.6. (master...msvc_qtupdate) https://github.com/bitcoin/bitcoin/pull/17727
< bitcoin-git> [bitcoin] MarcoFalke reopened pull request #17727: WIP: Update msvc build to ignore warning and bump Qt version to 5.12.6. (master...msvc_qtupdate) https://github.com/bitcoin/bitcoin/pull/17727
< wumpus> heh importing remote desktop services doesn't really make me happy either
< sipsorcery> wumpus: I'll check the msvc build with #17730, hopefully the Wtsapi32.lib can be removed.
< gribble> https://github.com/bitcoin/bitcoin/issues/17730 | depends: remove Qt networking features by fanquake . Pull Request #17730 . bitcoin/bitcoin . GitHub
< wumpus> thanks!
< wumpus> I really like PEP 540, 'ignore locale settings, just make everything UTF-8 already'
< wumpus> everyone gets the locale wrong on UNIX platforms anyway
< wumpus> time to just standardize on UTF-8 and not look back...
< bitcoin-git> [bitcoin] laanwj pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/3f1966ead6b2...3914e877c476
< bitcoin-git> bitcoin/master 2bcf1fc Pieter Wuille: Pass a maximum output length to DecodeBase58 and DecodeBase58Check
< bitcoin-git> bitcoin/master 5909bcd Pieter Wuille: Add bounds checks in key_io before DecodeBase58Check
< bitcoin-git> bitcoin/master 3914e87 Wladimir J. van der Laan: Merge #17511: Add bounds checks before base58 decoding
< bitcoin-git> [bitcoin] laanwj merged pull request #17511: Add bounds checks before base58 decoding (master...201911_boundedbase58) https://github.com/bitcoin/bitcoin/pull/17511
< bitcoin-git> [bitcoin] laanwj pushed 9 commits to master: https://github.com/bitcoin/bitcoin/compare/3914e877c476...0192bd065230
< bitcoin-git> bitcoin/master fd9d6ee Andrew Chow: Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage
< bitcoin-git> bitcoin/master e576b13 Andrew Chow: Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey()
< bitcoin-git> bitcoin/master 97c0374 Andrew Chow: Move Unlock implementation to LegacyScriptPubKeyMan
< bitcoin-git> [bitcoin] laanwj merged pull request #17369: Refactor: Move encryption code between KeyMan and Wallet (master...wallet-box-pr-4) https://github.com/bitcoin/bitcoin/pull/17369
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/0192bd065230...75a2a4f35744
< bitcoin-git> bitcoin/master fabd5b4 MarcoFalke: ci: Use python 3.7 on Windows Github Actions
< bitcoin-git> bitcoin/master 75a2a4f fanquake: Merge #17726: ci: Use python 3.7 on Windows Github Actions
< bitcoin-git> [bitcoin] fanquake merged pull request #17726: ci: Use python 3.7 on Windows Github Actions (master...1912-ciGitHubWindows) https://github.com/bitcoin/bitcoin/pull/17726
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/75a2a4f35744...8a01450b642a
< bitcoin-git> bitcoin/master fab2f35 MarcoFalke: doc: Update release process with latest changes
< bitcoin-git> bitcoin/master 8a01450 fanquake: Merge #17598: doc: Update release process with latest changes
< bitcoin-git> [bitcoin] fanquake merged pull request #17598: doc: Update release process with latest changes (master...1911-docRelNotes) https://github.com/bitcoin/bitcoin/pull/17598
< bitcoin-git> [bitcoin] brakmic opened pull request #17732: test: check for valgrind presence and set appropriate exit flags (master...valgrind-detection) https://github.com/bitcoin/bitcoin/pull/17732
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/8a01450b642a...cf2f43987696
< bitcoin-git> bitcoin/master 034561f Harris: cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice
< bitcoin-git> bitcoin/master cf2f439 fanquake: Merge #17687: cli: fix Fatal LevelDB error when specifying -blockfilterind...
< bitcoin-git> [bitcoin] fanquake merged pull request #17687: cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice (master...blockfilter-index-error) https://github.com/bitcoin/bitcoin/pull/17687
< bitcoin-git> [bitcoin] brakmic opened pull request #17735: build: fix typo (master...github-workflow) https://github.com/bitcoin/bitcoin/pull/17735
< wumpus> this almost has to be the OOM killer right ? #17733
< gribble> https://github.com/bitcoin/bitcoin/issues/17733 | bitcoin-qt does not start, terminates with "Killed" and exit code 137 . Issue #17733 . bitcoin/bitcoin . GitHub
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/cf2f43987696...54e11a39e14d
< bitcoin-git> bitcoin/master 5096baf Harris: build: fix typo
< bitcoin-git> bitcoin/master 54e11a3 MarcoFalke: Merge #17735: ci: fix typo
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #17735: ci: fix typo (master...github-workflow) https://github.com/bitcoin/bitcoin/pull/17735
< wumpus> oh noo you broke #17727
< gribble> https://github.com/bitcoin/bitcoin/issues/17727 | WIP: Update msvc build to ignore warning and bump Qt version to 5.12.6. by sipsorcery . Pull Request #17727 . bitcoin/bitcoin . GitHub
< fanquake> yea I'm not sure why 17735 was merged
< fanquake> Could have at least asked for a proper commit message as well
< fanquake> As for 17727, if we need to "make appveyor green", can we pull out the part of that PR that will actually make that happen. Rather than rushing through a qt bump that pulls in a bunch of new dependencies.
< wumpus> the problem is that the old qt vcpkg is not built with the new version of msvc
< wumpus> so it forces a qt bump indirectly...
< fanquake> ugh
< wumpus> but yeah we'd never let travis determine our dependency versions for the autotools build system
< wumpus> we could give up on appveyor, I think sipsorcery prefers moving to a github action as well
< wumpus> getting red crosses for every commit is getting kind of annoying so I understand the desire to make it green
< wumpus> also *other* changes that break MSVC will now sneak through
< fanquake> Sure. It's very annoying. The other concern is that we tell people to use the precompiled qt binaries to test things on windows, while at the same time are supposedly going to merge changes to the windows (MSVC) build system without even testing/checking them.
< wumpus> in some ways a faulty CI is worse than no CI at all
< wumpus> well there's a difference in maturity there: the precompiled binaries are the only supported ones, the MSVC build system is extra and experimental
< wumpus> it doesn't have nearly the amount of testing the gitian-built executables have
< fanquake> I'd be happy if just dropped support for all the "other" ways to build on windows aside from the Ubuntu cross-compiling
< wumpus> I think the only reason to support MSVC is to give windows developers an easier way to build and contribute
< wumpus> not to actually se MSVC-built bitcoin in production
< wumpus> also testing with another compiler has turned up some bugs in the past
< wumpus> which is a sensible reason to do a CI run with it
< wumpus> (though, this worked just as well without qt)
< fanquake> Yes, the introduction of the precompiled qt is where potential issues are.
< fanquake> One being that if no one is going to bother to check the precompiled binaries, while at the same time, windows build system changes are pushed through with no review just to make the CI green. I'm imaging a worst case scenario where it turns out the new qt is malicious, and coupled with something like linking against the Remote Desktop API, bad things may happen.
< fanquake> Obviously we do call out that the qt binaries should never be used with funds, testing only etc etc
< wumpus> at some point you start to wonder how much time is spent making linters, parallel build systems, CI, etc happy versus solving actual end user issues :)
< wumpus> yes, good point w/ pre-built qt
< fanquake> > at some point you start to wonder how much time +1
< sipsorcery> in the recent past it's been a LOT of time to sort out msvc CI issues (on top of the time PR authors spend looking into CI failures)
< sipsorcery> the Qt version bump is a separate issue. Concerns about switching to a new version, even for experimental builds, are fair.
< wumpus> so we might want to temporarily (or permanently, to replace with github actions) disable appveyor instead
< wumpus> instead of doing the qt upgrade just because appveyor forces us to
< sipsorcery> well appveyor didn't force a Qt upgrade, it forced a Qt recompile, and I was seeking to kill two birds with one stone.
< sipsorcery> github actions is faster and for that reason it'd get my vote to replace appveyor.
< sipsorcery> the advantage travis has had over appveyor, in terms of stability, is the dependencies being locked down
< sipsorcery> if the installed vcpkg libraries were built and cached externally to the job (same as Qt currently is) it would make them equivalent.
< ryanofsky> dependencies not being locked down could also be an advantage if we want to know about upcoming breakages
< ryanofsky> wouldn't it be easy to just ignore all appveyor errors in pull requests if appveyor on master is also broken?
< sipsorcery> true but to date it's been about a 50% (guess) hit rate on dependency changes being a genuine issue as compared to the package being tinkered with
< sipsorcery> e.g. the libevent package build was changed to disable threads by default which broke the appveyor build.
< ryanofsky> sipsorcery, was the libevent problem something appveyor specific that wouldn't affect someone who was trying to use msvc?
< sipsorcery> no it wasn't appveyor specific
< sipsorcery> anyone following the instructions and using vcpkg to install libevent would have had the same issue
< sipsorcery> appveyor updating the build image to use a new version of Visual Studio is the same thing.
< sipsorcery> anyone who currently attempts to build bitcoin core with Visual Studio 2019 v16.4 will get a compilation error.
< ryanofsky> yeah so all of that stuff seems useful to know about and fix as long as there is an msvc build. and i think it's ok if PR authors see red if their PR breaks the MSVC build
< ryanofsky> but i don't think PR authors should see red if master is also red
< ryanofsky> because that is much more likely to be wasting their time
< sipsorcery> yeah it'd be nice if the build icon could show yellow or orange if master is broken
< sipsorcery> i doubt there's a hook in GitHub to control that though
< ryanofsky> it's another thing we could ask for an as a workaround (though i'd prefer grey to really be able to ignore it)
< ryanofsky> as a workaround, maybe if appveyor is failing on a pull request, have it retry the build on master and just show green if both are broken
< bitcoin-git> [bitcoin] sipsorcery closed pull request #17727: WIP: Update msvc build to ignore warning and bump Qt version to 5.12.6. (master...msvc_qtupdate) https://github.com/bitcoin/bitcoin/pull/17727
< sipsorcery> moneyball: #proposedmeetingtopic appveyor CI future
< wumpus> the thing is, CI should be reliable, yes dependenies not being locked down shows about upcoming breakages but that's not something you want to be confronted with while focusining on other things
< wumpus> it's intrusive and that's annoying, we should set our own priorities based on what is merged and not have it break randomly "oh now you need to stop what you're doing and fix another CI isuse"
< luke-jr> can we set some CI instances to only warn?
< luke-jr> I guess ultimately, issues with new deps is really a master/branches-only thing, not PRs
< wumpus> I don't particularly want all new PRs to fail because there happens to have been a MSVC version that adds a new warning (which wouldn't even be fatal to normal builders)
< luke-jr> maybe it's a bad fit for CI
< luke-jr> or can CI instances be only for master/stable branches maybe?
< wumpus> failing on master it's annoying because it causes *all* PRs to fail, irrespective of that they do
< luke-jr> not if it doesn't run for PRs?
< wumpus> if it doesn't run for PRs it's pointlessbecause you don't know when it legitimately breaks the build
< luke-jr> ?
< luke-jr> I mean have the current CI instances with locked down versions, and new master/stable-only instances that use the latest..
< wumpus> the idea of doing a MSVC run is that you can be sure that a PR will work with MSVC, you don't only want to discover that after merge
< luke-jr> I mean, we either run PRs with latest versions or we don't..
< wumpus> sure, people are free to run master on whatever platform / library / compiler combinations they want
< luke-jr> I suppose if we manually check for and fail on warnings, we could exclude ones seen on master
< sipa> there is also something to be said about warnings/errors from msvc forcing us to maintain better code
< sipa> but if it's a pain to maintain the CI for it to get a good signal, that's probably not worth the effort
< wumpus> I don't think anyone is arguing not doing a MSVC build, but it should be stable dependency-wise like the travis one
< wumpus> all in all we've spent way too much time with CI broken lately, and only a few of those things were due to actual problems introduced in PRs
< luke-jr> wumpus: so why can't we do that, and add a master-only unstable build so we get alerted to new issues?
< jeremyrubin> I don't have anything technical to add; but I find it really annoying because it takes up a lot of mental space to get the "build broken" emails just for it to be another dumb msvc issue
< wumpus> jeremyrubin: same
< jonatack> agree. when we see 2-3 maintainers and several contributors spending a considerable amount of their time dealing with ci issues i'm thinking of the opportunity cost (and motivation cost)
< jonatack> make that 4 maintainers actually, just remembered bitcoinbuilds.org
< luke-jr> I think there's probably unanimity on that :P
< wumpus> heh
< ryanofsky> i think the compromise for appveyor i was suggesting (show red on a PR if the PR appveyor fails and master passes, green if both pass or both fails) has the best of all worlds
< ryanofsky> it gives the windows maintainer time to fix problems if anything breaks, while not bothering anybody else, and still showing red if a PR would break the windows build
< wumpus> yes, I guess "it's not getting worse" as a measure would work
< jeremyrubin> One thing that's annoying about that in terms of diagnosing errors is that given how checkered failures are, there ends up being an assumption of being rebased on master
< wumpus> though it could still happen, if the dependency drift still happens, that they change between the run on master (whichwas still succesful) and the PR
< wumpus> that assumption is already there, AFAIK the CI always runs on the PR merged into master
< ryanofsky> wumpus, i think that could be addressed by writing the appveyor script as: (git checkout pr && make check) || (git checkout master && ! make check)
< wumpus> ryanofsky: yes, that would work (though mind, appveyor is already slower than travis)
< wumpus> that's why some want to switch to github actions
< wumpus> (while others, in the past, have expressed to not want to use github actions, for ex. because it is even more vendor lock-in to github)
< wumpus> but for just the MSVC build it'd not be that bad, I think
< bitcoin-git> [bitcoin] sipsorcery opened pull request #17736: Update msvc build for Visual Studio 2019 v16.4 (master...msvc_fix_vs2019) https://github.com/bitcoin/bitcoin/pull/17736
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Dec 12 19:01:20 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< emilengler> hi
< moneyball> hi
< amiti> hi
< sipa> hi
< sipsorcery> hi
< wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo
< wumpus> 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 jeremyrubin lightlike emilengler jonatack
< fanquake> hi
< jeremyrubin> hi
< jonatack> hi
< fjahr> hi
< wumpus> one proposed topic for this week in https://gist.github.com/moneyball/071d608fdae217c2a6d7c35955881d8a : appveyor CI future
< fanquake> cc sipsorcery
< achow101> hi
< sipsorcery> my suggestion is to switch from appveyor to github actions (which is Azure CI under the hood) purely for speed
< emilengler> I personally think we should use Cirrus CI only as it supports Windows, Linux, macOS and also FreeBSD
< wumpus> sorry, that was not the start of the topic
< wumpus> this is still the "propose your topics" phase of the meeting, then we'll start with high priority for review
< gwillen> I am hoping to get some feedback this week on #17717
< gribble> https://github.com/bitcoin/bitcoin/issues/17717 | Proposed PSBT sign/broadcast flow . Issue #17717 . bitcoin/bitcoin . GitHub
< kanzure> HI
< emilengler> oh sorry
< sipsorcery> oops sorry
< wumpus> gwillen: ok, let's addd that as topic then
< wumpus> #topic High priority for review
< jeremyrubin> #proposedmeetingtopic finish up #12763
< gribble> https://github.com/bitcoin/bitcoin/issues/12763 | Add RPC Whitelist Feature from #12248 by JeremyRubin . Pull Request #12763 . bitcoin/bitcoin . GitHub
< wumpus> https://github.com/bitcoin/bitcoin/projects/8 8 blockers, 7 chasing concept ACK
< jeremyrubin> oh sorry *not* high priority
< wumpus> anything to add/remove or that is ready for merge?
< achow101> #17537 for hi prio
< gribble> https://github.com/bitcoin/bitcoin/issues/17537 | wallet: Cleanup and move opportunistic and superfluous TopUp()s by achow101 . Pull Request #17537 . bitcoin/bitcoin . GitHub
< nehan> hi
< wumpus> achow101: for blockers I guess?
< achow101> yes
< wumpus> ok, added
< wumpus> #topic appveyor CI future (sipsorcery)
< emilengler> Like I said, I personally think we should use Cirrus CI only as it supports Windows, Linux, macOS and also FreeBSD
< wumpus> sipsorcery | my suggestion is to switch from appveyor to github actions (which is Azure CI under the hood) purely for speed
< sipsorcery> second suggestion is to cache the vcpkg dependencies in a .zip file to avoid failures unrelated to PR's.
< luke-jr> emilengler: ppc64?
< emilengler> luke-jr: I'm not sure about that but we keep using travis for this purpose only
< wumpus> I don't think MarcoFalke is here
< emilengler> could keep*
< wumpus> is anyone against moving to github actions for just the MSVC build?
< luke-jr> emilengler: what about distros?
< wumpus> (there is no question of moving *all*CI to github actions, to be clear)
< jeremyrubin> wumpus: given that github is a microsoft product seems reasonable :p
< wumpus> jeremyrubin: heh!
< fanquake> As long as that involves removing all Appveyor usage.
< emilengler> luke-jr: I'm not sure about distros
< wumpus> fanquake: yes, it would
< emilengler> But anyway, I believe that we should use Cirrus CI in the long term. Maybe only for FreeBSD?
< emilengler> But that's anotehr topic
< wumpus> fanquake: this would be about replacing appveyor
< fanquake> wumpus great. If it's faster, and isn't going to be broken seemingly every thrid day for reasons outside our control.
< wumpus> I can't comment on that
< fanquake> Maybe the vcpkg maintainers will stop randomly slicing up their packages.
< MarcoFalke> I got the appveyor timeout back to 90 minutes
< MarcoFalke> Anything else needs to be fixed?
< sipa> but is the issue with appveyor itself, or the fact that vcpkg changes out from under us all the time?
< jeremyrubin> My understanding was that it's vcpkg
< wumpus> both, appveyor is slow and has a low timeout, and the packages switch under us all the time
< jeremyrubin> But that actions will make it less annoying for non windows maintainers
< fanquake> There's a good summary of some of the issues in #17736
< gribble> https://github.com/bitcoin/bitcoin/issues/17736 | Update msvc build for Visual Studio 2019 v16.4 by sipsorcery . Pull Request #17736 . bitcoin/bitcoin . GitHub
< sipa> okay, so there are two independent issues to be fixed
< fanquake> 3 if you include the compiler issues.
< wumpus> yes, I'm not sure in how far they're correlated
< ryanofsky> well my earlier suggestion for dealing with vcpkg issues will not work if appveyor is too slow
< sipsorcery> at a guesstimate on appveyor failures: 5% genuinely relate to PR's, 30% vcpkg, 25% python functional tests, 5% appveyor image, 30% appveyor timeout, 5% other.
< fanquake> a literal roulette wheel
< sipsorcery> suspect GitHub, Cirrus or Circle could all be the same excepting the timeouts.
< sipsorcery> assuming similar reliability, my vote would go to whichever one is fastest
< wumpus> yes
< sipsorcery> GitHub is substantially faster than appveyor. I haven't tested any other but would be happy to do so if it would help?
< luke-jr> speed seems likely to be dependent on how many projects are using them
< luke-jr> ie, if we switch, it will get slow :P
< wumpus> fair enough, our test suite is pretty heavy!
< wumpus> sipsorcery: let's just try github next imo
< wumpus> sipsorcery: no one was strongly against trying them
< MarcoFalke> What about #17736 ?
< gribble> https://github.com/bitcoin/bitcoin/issues/17736 | Update msvc build for Visual Studio 2019 v16.4 by sipsorcery . Pull Request #17736 . bitcoin/bitcoin . GitHub
< wumpus> #17736 would be necessary no matter the CI I think
< gribble> https://github.com/bitcoin/bitcoin/issues/17736 | Update msvc build for Visual Studio 2019 v16.4 by sipsorcery . Pull Request #17736 . bitcoin/bitcoin . GitHub
< sipsorcery> if appveyor is being canned 17736 is not required (at least until GitHub upgrade).
< wumpus> oh
< MarcoFalke> Has anyone tested the GitHub CI and found any issues?
< MarcoFalke> I merged it a few days ago
< fanquake> I didn't realise that Github seems to automatically turn the CI on for any repos that have the config file? Had to turn it off for my own fork.
< fanquake> Didn't realise until a build had failed and it emailed me.
< MarcoFalke> Huh
< MarcoFalke> It was turned off for bitcoin/bitcoin
< sipa> fanquake: maybe you're part of a weird opt in program?
< MarcoFalke> ^
< fanquake> Yea I'm not talking about bitcoin/bitcoin. Which we had tried to turn "Actions" off for. This was one my fanquake/bitcoin fork.
< fanquake> sipa could be
< wumpus> someone, not me at least turned 'actions' back on for bitcoin/bitcoin (which is okay if we're going to use it)
< bitcoin-git> [bitcoin] jamesob opened pull request #17737: Add ChainstateManager, remove BlockManager global (master...2019-12-au.chainman) https://github.com/bitcoin/bitcoin/pull/17737
< fanquake> wumpus Yea I was wondering how that happened
< wumpus> in any case, let'stest it and talk about it next meeting or so
< MarcoFalke> Ok, so unless there are objections: Merge #17736 as short term fix, and then (next week maybe) switch to GitHub Actions CI?
< gribble> https://github.com/bitcoin/bitcoin/issues/17736 | Update msvc build for Visual Studio 2019 v16.4 by sipsorcery . Pull Request #17736 . bitcoin/bitcoin . GitHub
< sipsorcery> +1
< fanquake> +1
< wumpus> ack
< emilengler> +1
< wumpus> #topic Proposed PSBT sign/broadcast flow (gwillen)
< wumpus> #17717
< gribble> https://github.com/bitcoin/bitcoin/issues/17717 | Proposed PSBT sign/broadcast flow . Issue #17717 . bitcoin/bitcoin . GitHub
< sipa> yay, PSBT guis
< gwillen> this is building on top of Sjors' #17509 which is not merged yet
< gribble> https://github.com/bitcoin/bitcoin/issues/17509 | gui: save and load PSBT by Sjors . Pull Request #17509 . bitcoin/bitcoin . GitHub
< gwillen> my general sense is, once we can load PSBTs, the next step is "what are the operations we support on them"
< gwillen> so I think introducing a simple dialog to analyze them, sign them, broadcast them, and save them is a good next step
< gwillen> (this is simpler than the dialog from my old branch, for those of you who saw that, but it will reuse a lot of the parts)
< emilengler> ack
< sipa> i agree with the observation that an explicit update step is probably unnecessary
< gwillen> yeah my approach has always been to automatically update the PSBT eagerly when loading it, and that seems to produce good results
< achow101> seems reasonable
< gwillen> anyway I'm trying to make this a small and uncontroversial step, so feedback welcomed on the issue, there will be a PR soon, thanks!
< sipa> the only reason why you'd want that to be explicit is because of privacy concerns (you may not want to reveal bip32 paths you know about keys used), but the sentiment in #17264 seems to be that that needs to be solved differently anyway
< gribble> https://github.com/bitcoin/bitcoin/issues/17264 | rpc: set default bip32derivs to true for psbt methods by Sjors . Pull Request #17264 . bitcoin/bitcoin . GitHub
< gwillen> sipa: hmm *nod*, will go read the notes on that
< instagibbs> yeah making PSBT private is not super well defined to begin with, you need additional roles
< instagibbs> especially as more special-case fields are entered into it
< luke-jr> "update"?
< instagibbs> luke-jr, filling fields like hd derivation path, etc
< luke-jr> not signing, though, right?
< gwillen> luke-jr: it's possible to have a PSBT that doesn't have things like P2SH redeemscripts, witnessscripts, etc. that are required for signing
< gwillen> I would never have the UI sign automatically, no
< achow101> luke-jr: correct. update is done before signing
< sipa> luke-jr: BIP 174 updating is filling in UTXOs/prevtxs, bip32 paths, scripts used
< gwillen> thanks for noting that clarification
< gwillen> signing is always a big red button that's explicit
< luke-jr> hmm
< instagibbs> updating is done to "guide" signing, especially for dumb signers
< luke-jr> exposing redeemscripts is arguably partially a signature?
< gwillen> well the design of PSBT in Core is such that the wallet creating the tx will already include them if it knows them
< sipa> luke-jr: it doesn't involve private keys
< gwillen> the "update" step only ever occurs separately in unusual cases I think
< luke-jr> sipa: I'm thinking the script itself is part of the private key
< jb55> oh that's probably why I could never get rawtx to psbt conversion to work, I probably needed to call utxoupdatepsbt?
< instagibbs> that conversion is sketchy :)
< sipa> luke-jr: it should not be considered that way
< sipa> luke-jr: as security should not ever depend on keeping scripts private
< luke-jr> hmm
< sipa> (it may matter for privacy to keep them secret, but that's a much more complicated issue to solve if desired)
< gwillen> given that PSBT is pretty dependent on the current workflow, I think the workflow is maybe outside the scope of my specific change
< achow101> if you want privacy of scripts and derivations paths, psbt ain't it
< gwillen> but might be worth having a discussion about it at some point
< luke-jr> I guess the scripts are strictly less a concern than derivation paths
< jeremyrubin> sipa: it's sometimes possible to compress a HTLC in a taproot branch I think
< achow101> jeremyrubin: taproot will have it's own set of fields that hopefully better preserve privacy
< jb55> should the ui give a privacy warning when copying psbts to clipboard/saving when there's derivation paths included?
< instagibbs> you'll still need additional roles/flows imo
< sipa> jb55: imho, no - it's generally not actionable
< instagibbs> better to me to actually define common flows, and design for those, rather than just guessing
< sipa> jb55: you have the choice between private and broken, and non-private and functional
< gwillen> I think in general PSBTs are also never intended to be published, they are intended to be shared only with required signers
< instagibbs> hww<->host<->curious co-signer, things like that
< gwillen> I suspect this really mutes a lot of the privacy issues that could otherwise exist
< instagibbs> but there's no RPC flow for this
< gwillen> for most people the required signers will only ever be themselves
< achow101> gwillen: the main privacy concern is for coinjoins
< gwillen> or people they already trust with their privacy
< gwillen> *nod*
< gwillen> I have not thought a lot about the coinjoin case
< bitcoin-git> [bitcoin] fanquake opened pull request #17738: build: remove linking librt for backwards compatibility (master...remove_librt_back_compat) https://github.com/bitcoin/bitcoin/pull/17738
< instagibbs> depends on what co-signers require to sign, there can be additional proof data etc
< achow101> in coinjoins you don't know or trust the other participants. deriv path and scripts could provide an easy way for a coinjoin participant to deanonymize other coinjoin participatns
< sipa> yeah, the (interesting) privacy issues appear when you're combinging multi-party signing for some inputs, and distinct signers for other inputs
< sipa> there could be some best effort where we remove bip32 derivation info from finalized inputs
< gwillen> we may already do that
< achow101> pretty sure we do
< gwillen> I know that we strip partial signatures from finalized inputs once we combine them
< achow101> I think the spec for finalizer says drop everything but final sig/witness and utxo
< sipa> oh, ok
< sipa> achow101: i vaguely remember that indeed
< instagibbs> oh that's not too bad then for most uses already
< achow101> I think the workflow we should be going for is really the hardware wallet/cold storage one
< jb55> I still wish there was some hrp that I could glance at the various states a psbt is in, but I guess that ship has sailed
< instagibbs> well that one can be very naive
< gwillen> jb55: can you expand "hrp"
< achow101> human readable aprt
< achow101> *part
< gwillen> there is the analyzepsbt rpc which produces what you want, I think
< sipa> the stage depends on the input...
< gwillen> and my goal is for my UI to also expose as much of that as makes sense
< jb55> yeah perhaps thats just difficulties arising from copying and pasting psbts and the cli, but ui could help a lot here
< gwillen> I don't think it would make sense to put an hrp in the PSBT itself, you could attack people by having it be secretly out of sync with the rest
< instagibbs> jb55, different signers may expect different input, so it's kind of hard to say. e.g., one may expect xpub for multisig detection, other may not
< jeremyrubin> Anything else on this topic?
< gwillen> I think further comments on my proposal can go on the issue
< wumpus> #topic RPC Whitelist Feature (jeremyrubin)
< jeremyrubin> I think this is more or less ready to merge
< emilengler> The test (#17536) is also rebased on the squashed commit
< jeremyrubin> It adds the ability to whitelist certain RPCs for credentials for those unfamiliar
< gribble> https://github.com/bitcoin/bitcoin/issues/17536 | test: Add test for rpc_whitelist by emilengler . Pull Request #17536 . bitcoin/bitcoin . GitHub
< instagibbs> could you snipe the test? :) if that's ok emilengler
< emilengler> instagibbs: What do you mean?
< jeremyrubin> snipe meaning put onto the same branch? I could, I just didn't want to unfairly steal emilengler's merge stats ;)
< emilengler> jeremyrubin: It's fine
< instagibbs> emilengler, can jeremy cherry-pick into his branch? I don't like features without tests :)
< jeremyrubin> big credit to emilengler for putting in the effort to get the tests over the line
< emilengler> sure
< wumpus> just make sure to keep the author data the same
< instagibbs> +1
< emilengler> Would be nice if you colud put me into the longer commit description
< jeremyrubin> will do
< instagibbs> will review
< emilengler> thanks :)
< luke-jr> do it without an @ or he'll get spammed by altcoins
< emilengler> Will close the PR then
< luke-jr> (usually just maintaining the Author field is enough?)
< jonatack> will review
< instagibbs> 8 minutes, any other topics?
< wumpus> thanks everyone, I don't think we have any more topics so that concludes the meeting
< instagibbs> kk
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Dec 12 19:52:49 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< emilengler> Thanks everyone
< instagibbs> MarcoFalke, is there hidden meanings in your signed emojis on PRs :)
< MarcoFalke> instagibbs: ?
< bitcoin-git> [bitcoin] instagibbs closed pull request #17712: sendmany/bumpfee: Avoid address reuse and partial spends as per walle... (master...avoid_reuse_sm_bf) https://github.com/bitcoin/bitcoin/pull/17712
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/54e11a39e14d...c5e318aea6ad
< bitcoin-git> bitcoin/master 75d9317 Aaron Clauson: Update msvc build for Visual Studio 2019 v16.4
< bitcoin-git> bitcoin/master c5e318a MarcoFalke: Merge #17736: Update msvc build for Visual Studio 2019 v16.4
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #17736: Update msvc build for Visual Studio 2019 v16.4 (master...msvc_fix_vs2019) https://github.com/bitcoin/bitcoin/pull/17736