< 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.
< 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
< 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
< 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?
< 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
< 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