< harding>
lopp: under the default GitHub settings, anyone with commit access can push anything they want, so they could pull a PR into a private branch, modify it, merge it locally, and push it. Some projects enable GitHub settings that prevent direct pushes like that to the master branch, but I'm not sure whether Bitcoin Core uses them. Alternatively, when you open a GitHub PR, a box is checked by default that allows people with commit
< harding>
access to the target repository to push to the branch, which is another way comitters could modify the code. What comitter Mallory can't do is change contributor Alice's commits without removing any PGP signature she put on them, so if you trust Alice's key (and if she always signs her commits), you can confirm she provided those commits.
< midnightmagic>
code review would need to be compromised; and all the people who build it would have to miss it; and then following that the users upgrading would similarly have to miss it. wumpus does some weird things to double-check the merge process including comparing self-made branches with the PR results and so on.
< midnightmagic>
lopp: anything not attached to an actual process including PR and discussion would be noticed (IMO) fairly rapidly and examined more closely.
< lopp>
midnightmagic: it's mainly the "maintainer slips in a few lines to a PR" vector I'm trying to better understand
< midnightmagic>
lopp: If I were a PR submitter, I would notice that. And the commits are tracked well, so even e.g. push -f could be easily forensically examined. Many of us keep full history immune to push -f tip disconnection.. A PR mod would be an obvious slip.
< midnightmagic>
lopp: external verification of correct merging would likely be welcome though, if you have a script or something that you want to implement. Well. I mean I'd run it anyway.
< sipa>
midnightmagic: i think a force push to the repo would be easily noticed yes
< sipa>
i'm not so sure about a PR being modified by the maintained
< midnightmagic>
Maybe that's just me then. But authorship would be contaminated at that point. I as a PR submitter would have noticed instantly if you guys committed something other than what I PR'd.
< promag_>
midnightmagic: how?
< gmaxwell>
midnightmagic: I'm fairly confident that almost no submitters would notice a subtly different version.
< gmaxwell>
A grossly different version, sure.
< gmaxwell>
Esp since we don't force PRs to be rebased by the submitter before merging them.
< midnightmagic>
weird.
< gmaxwell>
(I mean, they usually have to apply cleanly, but thats it)
< gmaxwell>
many reviewers ack the specific merged commit, but that data is just lost in github land.
< promag>
gmaxwell: never saw that (I think)
< gmaxwell>
At least if the modified the change inside the merge commit, that would be apparent in the git history.
< midnightmagic>
thus authorship contamination.
< sipa>
no, changes in the merge commit wouldn't change the commit that comes from the author
< sipa>
the commit id would be exactly the expected value
< gmaxwell>
sipa: right, but it would be apparent in the git history that the merge wasn't faithful, if anyone looked.
< gmaxwell>
(I'm not saying that anyone looks)
< midnightmagic>
the commit of the submitter could but the merge commit wouldn't.
< promag>
gmaxwell: you said "many reviewers ack the specific merged commit"
< sipa>
midnightmagic: i'm confused what you're referring to
< promag>
I thought you meant the merge commit
< gmaxwell>
promag: sorry, I didn't make the clear, no not the merge commit unfortunately that can't really happen logistically. :(
< gmaxwell>
promag: but the commit that is eventually merged, not the merge commit itself.
< sipa>
we actually could very reasonable verify that merge commits are faithful and conflictless
< sipa>
but we don't have infrastructure for that
< sipa>
it would be kinda pointless without also verifying e.g. that the author commit matches the value that was ACKed by reviewers etc
< gmaxwell>
Someone could write automation that checks that merge commits are faithful, but I would WAG that not 100% of them are.
< midnightmagic>
sipa: those commitids which are added to git history as a result of the merge would include a change by the maintainer, assuming we have a commit in the history which the PR submitter actually intended to be committed. Changes would be visible via git log and other historical examination
< gmaxwell>
sipa: well the merge commit could copy existant acks from the github into the commit message.
< gmaxwell>
The point I was trying to make and midnightmagic is making is that at least if it's the merge commit that screws it up there is a clear record.
< sipa>
midnightmagic: yeah, it would be - i'm just doubtful that there is any infrasturcture or reviewer policy that would notice that
< promag>
midnightmagic: do you periodically check history and merge commits?
< sipa>
midnightmagic: because git doesn't tell you unless you go out of your way
< gmaxwell>
vs editing the commit that gets merged itself, which would actually not leave a good record.
< promag>
the same regarding merge os backports
< midnightmagic>
sipa: I accept that. I'm just saying I would definitely notice contamination of any PR I submitted. And I think it's weird that other submitters don't verify. :(
< promag>
s/os/of
< gmaxwell>
midnightmagic: how would you even go check?
< gmaxwell>
I guess do the merge yourself and compare the resulting trees?
< sipa>
git show on the merge commit will show you the conflicts, i think
< sipa>
(but it does depend on your local diff/conflict resolving strategy, which is a config setting)
< midnightmagic>
git log, diffs, looking at the merge points and reverifying actual code committed. I don't really trust git either, after watching superior commercial systems screw up their merging in corner cases that wrecked large corp IP.
< gmaxwell>
I'm no longer a commiter on bitcoin, I'd be interested in knowing how often commiters are manually resolving conflicts on master. I believe the answer is rarely but not never.
< sipa>
gmaxwell: i think the answer is almost never
< gmaxwell>
I think I recall doing so... but only like ... once.
< promag>
gmaxwell: never noticed one in almost 2 years
< sipa>
promag: how would you notice?
< gmaxwell>
because our history is not linear there is virtually always conflict resolution, but it's automated.
< promag>
right, I mean, it wasn't discuessed here at least XD
< gmaxwell>
promag: yea, it wouldn't get discussed. like as a commiter I would have only done a manual resoution if it was utterly trivial and obvious, and otherwise I would have nagged someone to rebase.
< sipa>
i personally would always ask for rebasing instead of merging something with conflicts, but i'm not using my merge power frequently
< sipa>
and if that doesn't happen, submit a PR myself with the rebase
< gmaxwell>
sipa: even if the PR author had been gone for two months ?
< gmaxwell>
okay fair enough.
< sipa>
i think in backport branches maintainer changes may be more common, but i shouldn't speak for others
< * midnightmagic>
discovers taking something for granted.
< gmaxwell>
Actually I think I've seen you do exactly that, open a rebase basically right before something gets merged.
< gmaxwell>
sipa: yea, backports are a mess of manual resolution, thats why I was specific about master.
< gmaxwell>
In any case, I expect we could pretty easily have a 'no manual resolution in merges' standard, but its kinda pointless without validation.
< gmaxwell>
and I wouldn't be surprised if validation might be complicated by git version differences. :(
< promag>
I think backports are more attractive to tamper, are on the verge of release
< gmaxwell>
maybe. but they inherently get a lot less attention in a lot of regards.
< gmaxwell>
Tampering in general is not the most attractive attack vector because its conspicuous. I think in general when we think about process improvements, it's best to think in terms of beneficial changes that make tampering harder as a side effect, rather than targeted changes against tampering.
< bitcoin-git>
bitcoin/master 0e75f44 Pieter Wuille: Replace CAffectedKeysVisitor with descriptor based logic
< bitcoin-git>
bitcoin/master 7a30e0f MeshCollider: Merge #14821: Replace CAffectedKeysVisitor with descriptor based logic...
< bitcoin-git>
[bitcoin] MeshCollider closed pull request #14821: Replace CAffectedKeysVisitor with descriptor based logic (master...201810_die_caffectedkeysvisitor_die) https://github.com/bitcoin/bitcoin/pull/14821
< midnightmagic>
lopp: check the merge commits. unclean commits result in diffs of the merging commit and the topmost merge point; clean merges result in a pure list of commits between the branchpoint of the PR branch and the topmost commit. Alternatively, for a forced merge commit (like what github does) you can diff the base of the merge and the commitid of the merging commit and the results should be
< sipa>
midnightmagic: still a merge commit above it; 024816d6cfc719c7109cf5f1aa02d41056df848d
< sipa>
no?
< sipa>
oh no
< aj>
contrib/verify-commits.py has code to check merge commits are clean... there's an explicit list of non-clean merge commits as well (contrib/verify-commits/allow-unclean-merge-commits)
< sipa>
you're right; hmm
< meshcollider>
historical release notes are the only time I've seen a direct push to master IIRC
< sipa>
ah, yes, there can be direct commits in masters, but they need to be signed by a maintainer
< sipa>
and merge commits, which also need to be signed
< gmaxwell>
aside, github output on the commit is weird, look what PR it attributes it to
< sipa>
and merge commits cannot merge more than 2 branches
< gmaxwell>
no octopus merges? aww
< midnightmagic>
:-o
< meshcollider>
yep thats enforced by verify-commits
< gmaxwell>
I wonder if we shouldn't just have the verify also disallow direct commits on master. It doesn't much matter one way or another, since by norm they're not used (except for trivial things).
< gmaxwell>
also not terribly important one way or another, I guess.
< gmaxwell>
but the fact that sipa and I thought they weren't allowed maybe suggests that they shouldn't be.
< midnightmagic>
"you man. don't eat those."
< meshcollider>
its exactly the same as opening a PR and then merging it straight away, I don't think there is any security difference
< meshcollider>
both will be alerted on IRC by the commit bot for example
< gmaxwell>
meshcollider: indeed, but in terms of process flow, it's one fewer things to watch out for.
< gmaxwell>
::shrugs:: I don't have a strong opinion, but given that we almost never do it, it might be better to never do it.
< gmaxwell>
If nothing else, it helps draw a stronger line between our practices and common but bad practices (lots of cryptocurrency projects commit everything directly to their repo with no review or oppturnity for review at all)
< gmaxwell>
obviously its up to the people who do it. :)
< sipa>
yeah, it's easy to discuss policies when you're not the person affected by them :)
< gmaxwell>
Well, I am affected by them, at least in theory. I give at least a glance over look at _every_ commit in a release. If I were doing that by going through the list on github, then I'd miss some.
< gmaxwell>
In actuality, I do it via git, so I wouldn't ... but I could have been! :P
< midnightmagic>
ha
< gmaxwell>
(looking at changes on github is kind of a lost cause because of the automatic file hiding, among other things)
< meshcollider>
gmaxwell: do you mean the list of PRs merged, or the list of commits itself
< meshcollider>
because of course direct commits to master will be in the latter
< gmaxwell>
meshcollider: If I had moved to doing the review via github, I would have been doing it on PRs.
< meshcollider>
fair enough
< gmaxwell>
also with the mistaken impression that there was a Pr for everything.
< aj>
if nothing else, doing everything via a PR provides an easy place to capture comments on commits
< gmaxwell>
aj: right thats why I would use PRs if I were doing it via the web.
< gmaxwell>
In reality, I prefer to not see the comments.
< gmaxwell>
because they can be misleading.
< gmaxwell>
(A lesson I learned on this project a couple years back...)
< aj>
gmaxwell: comments are helpful if you're looking at code that was merged a while ago, and want to understand some of the choices. in theory, anyway...
< gmaxwell>
aj: what I like to do is look just at git, and then if I can't understand it, go look at the PR.
< aj>
gmaxwell: yeah, exactly
< gmaxwell>
less loading of expectations.
< aj>
gmaxwell: (then find i can't understand it after reading the PR, then ask on irc :)
< gmaxwell>
I couple times I've caught bugs by reading the change instead of the PR that I'm confident that I wouldn't have caught if I read the PR first.
< gmaxwell>
(though for unmerged things I still end up always reading the PR first, ... lazy oh well)
< luke-jr>
(#11082 is rebased btw)
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
< luke-jr>
(if someone wants to add it to the high-prio list..)
< meshcollider>
luke-jr: done
< lopp>
sipa: so is it accurate to say that maintainers can't alter commits that they merge because it would cause the verify-commits script to fail?
< promag>
lopp: what if they change verify commits or related?
< gmaxwell>
lopp: no that isn't the case at all.
< gmaxwell>
lopp: they can change the commits they merge, but it won't agree with the commit ids that people ack on PRs. Nothing automated checks this.
< gmaxwell>
lopp: parties other than the maintainers (e.g. github) can't do that though, because merge commits are signed and verify commits checks the signatures.
< gmaxwell>
promag: instructions on verify commits has you use it in a way where you fetch changes and then verify them before merging into your local tree.
< lopp>
gotcha; I know there is a trust issue with the verify-commits script itself though I'm not familiar with how the CI runs it... hopefully it doesn't run whatever version is at HEAD
< gmaxwell>
see the readme on verify commits "Using verify-commits.py safely"
< promag>
gmaxwell: still don't understand.. a merge commit could remove/skip the check in travis right?
< gmaxwell>
I have no idea what travis does. verify-commits is run by users (and should be run by you too), and if run like the readme says, no commit can defeat it.
< sipa>
promag: if you mean that someone can modify the verify-commits script to be neutered... sure, but i'm sure that would be noticed by review
< promag>
gmaxwell: thanks, will do!
< promag>
meh, trust in maintainers \o/
< sipa>
promag: s/maintainers/reviewers/
< luke-jr>
lopp: don't trust CI
< meshcollider>
sipa: in #14565 is there a reason you moved pubkey_map and privkey_map outside of import_data
< gmaxwell>
sipa: re 14955 on the strenghtening in the scheduler, do you hold exclusive access the whole time, or do you get randomness, strenghten and then mix back?
< sipa>
gmaxwell: nope!
< sipa>
only at the end
< sipa>
the process is [gather entropy externally] [get entropy from local RNG] [strengthen] [feed back into local RNG]
< gmaxwell>
but it isn't holding any locks during strengthen? right?
< gmaxwell>
lol maybe you should use sha256d64 as the strengthening function. :P
< gmaxwell>
lot more strenghtening per unit time to do 4-parallel sha256d64. :)
< sipa>
gmaxwell: no, just when reading the local RNG, and when mixing back in
< sipa>
gmaxwell: perhaps!
< gmaxwell>
sipa: is there any way to arrange things so that no strong randomness will get gotten until one strengthening pass has happened?
< sipa>
gmaxwell: yes, already done
< gmaxwell>
sweet.
< sipa>
the first invocation will run with 'startup' seed level, which includes everything else
< gmaxwell>
we should like serialize out a random number on stop, and read it back in on start. (is there an existing dat file we could abuse for this, like peers or mempool :P ) so as to conserve our strenghtening efforts.
< gmaxwell>
Anyone heard any feedback on 0.17.1rc1?
< gmaxwell>
BlueMatt: ^ have you tested it to make sure it's all happy on ubuntu?
< gmaxwell>
warren: ^ have you tested it to make sure its all happy on the latest fedora?
< bitcoin-git>
[bitcoin] bpay opened pull request #14956: [qt] Ensure tx send error highlight is visible (master...show-highlighted-field) https://github.com/bitcoin/bitcoin/pull/14956
< fanquake>
gmaxwell trying to collect test feedback in #14902
< kallewoof>
Now that GetType() is gone in the source code, I'm not sure how to detect whether a serialization op is a hash operation or not. I need this for the block header in signet. Is there a new way to determine operation type somehow?
< sipa>
kallewoof: i don't see what you need it for
< sipa>
the hash should generally be the same as the network serialization
< kallewoof>
sipa: I include the signature when (de)serializing, but not when getting the hash. The signature is a payload of the block header, by necessity
< kallewoof>
I may be able to exclude the signature part when doing the actual sig check though. *checks*
< sipa>
kallewoof: that seems ugly
< sipa>
you can use a flag like the ones used for witness serialization
< sipa>
actually, you can use exactly the same flag
< kallewoof>
Oh!
< bitcoin-git>
[bitcoin] Empact opened pull request #14957: wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (master...stop-block-null) https://github.com/bitcoin/bitcoin/pull/14957
< fanquake>
Is anyone able to successfully compile on macOS with --with-sanitizers=thread ?
< gwillen>
fanquake: I ran into something like this, it's hard to recall the details but I think they may be in scrollback here
< gwillen>
IIRC, the resolution in my case is that OS X libtool is somewhat elderly and was stripping linker flags it didn't recognize, inappropraitely
< gwillen>
you can force libtool to pass through a flag by changing "-fblahblah" to "-XCClinker -fblahblah", or something to that effect
< gwillen>
I think you have to do that with -fsanitizer, assuming you're hitting the same issue I was
< gwillen>
which I faintly suspect you are because it's awfully similar
< wumpus>
so the reason that historical release notes are committed directly is that it is part of a long list of steps around a release, and other steps rely on it, for example creating a release on github has a link to the historical release notes in git which then need to have been committed; it's also simply a copy operation
< fanquake>
gwillen thanks, I'll check it out this arvo
< gwillen>
np, good lfuck
< gwillen>
er... luck*
< wumpus>
doesn't seem very useful to create a PR then as there's nothing to review, it's simply more work
< fanquake>
wumpus I've used GH's new (beta) issue "pinning" feature to pin #14902 & #14438 to the top of the issues page. Let me know if you object/can think of a third issue to add.
< fanquake>
I feel having the current testing issue, and release schedule pinned at the top can makes sense
< fanquake>
If anything maybe it'll draw more attention to testing rc1
< wumpus>
fanquake: makes sense, I think this is enough
< wumpus>
so effectively (#14948), the unit tests can only run now with a filesystem locale that supports fancy unicode characters, which is not the default on BSD; this wa already the case for the Python tests
< wumpus>
it's somewhat annoying but also understandable, I don't think anyone *intends* to run a pure ASCII locale and only support ASCII filenames on the other hand the assumption that the locale always matches UTF-8 might be incorrect
< wumpus>
(or is always able to represent all unicode characters)
< fanquake>
wumpus got to have wallet names/dirs full of emojis
< wumpus>
U+1F45B.dat
< fanquake>
heh
< wumpus>
that makes me wonder, though, how does git handle filenames with emojis, would it refuse to check out the tree on those platforms?
< wumpus>
or what about programs such as firefox? is "ignore the filesystem locale and assume it's always UTF-8" valid behavior for modern software?
< wumpus>
it changes the unknown unicode character to literal '????' oh my
< wumpus>
at least that's different from boost's handling which is to fail the conversion, not sure what's better...
< promag>
disallow that madness?
< wumpus>
like what? reject non-utf locales at startup?
< wumpus>
I mean, I guess ASCII file system locale simply works as long as you don't try to use non-ASCII characters in filenames
< wumpus>
which might be fine for English/US people
< promag>
I don't know, but it's a headache having wallet names from the filename
< wumpus>
that's why I wondered how other software handles this, but replacing iwht ???? doesn't seem that useful either...
< promag>
yeah, comes from "what the heck???? are you mad?"
< bitcoin-git>
[bitcoin] promag opened pull request #14958: qa: Remove race between conneting and shutdown on separate connections (master...2018-12-improve-shutdown-test) https://github.com/bitcoin/bitcoin/pull/14958
< provoostenator>
2. progrress towards hardware wallets
< jnewbery>
#11082 isn't wallety, but is blocking provoostenator's wallety PRs
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
< achow101>
hi
< meshcollider>
#topic high priority
< provoostenator>
jnewbery: it's blocking my future walety PR's (once I start on the GUI side of hardware wallets)
< meshcollider>
jnewbery: they are both already on the list right
< provoostenator>
Correct
< jnewbery>
14565 looks good. It was missing tests, but now looks pretty well covered (thanks sipa!)
< jnewbery>
reminder that 14565 blocks PRs from meshcollider and achow101, so it'd be nice to move it towards merge
< meshcollider>
Yes
< meshcollider>
I had one last little question on there
< meshcollider>
Is there anything else wallet related that should go onto high priority list now?
< meshcollider>
#topic progress towards descriptor wallets (provoostenator)
< * provoostenator>
looks as sipa
< provoostenator>
*at
< * sipa>
stares back
< instagibbs>
various descriptor support, like in listunspent?
< meshcollider>
sipa: have you thought more about it recently or been mostly focused on the PRNG stuff
< sipa>
meshcollider: no, sorry - i've been busy with a few other projects
< meshcollider>
that's fine of course :) I guess that is your update provoostenator
< meshcollider>
There was an issue tracking which RPCs to add descriptor support to iirc
< provoostenator>
Well, I'd like to know what's next, but we do have enough review work already I guess.
< sipa>
next step is moving IsMine and related functions to be part of the wallet or some other abstraction, rather than free functions
< sipa>
so they can later be extended to be descriptor based
< provoostenator>
Ok
< provoostenator>
Anything about the Keypool we can improve?
< meshcollider>
I can take a stab at the ismine stuff
< sipa>
that needs to be part of the same interface, i expect
< provoostenator>
For example I'm a bit worried about #14075 interacting with the keypool from RPC code.
< sipa>
but maybe a later step
< gribble>
https://github.com/bitcoin/bitcoin/issues/14075 | Import watch only pubkeys to the keypool if private keys are disabled by achow101 · Pull Request #14075 · bitcoin/bitcoin · GitHub
< sipa>
that looks like it may interact, indeed
< provoostenator>
Though it can be refactored after that, it's nice if we at least have an idea of what the final thing needs to look like.
< meshcollider>
So instead of the keypool being generated by a single descriptor, it will have imports in it too
< provoostenator>
Right now the keypool, when it expands itself, makes strong assumptions about the wallet and just uses the HD structure.
< provoostenator>
Whereas what we want probably is for the keypool (or something like it) to expand a specific descriptor.
< sipa>
well there won't be a keypool anymore
< sipa>
it's just a descriptor, which some entries cached, and some not (yet)
< sipa>
the hard part is integrating the existing logic into such a structure
< provoostenator>
Any prose or pseudo-code describing how such integration would work is most welcome (in a Github issue).
< sipa>
i think we'll want to see the existing keypool/keys/... as a special "legacy" descriptor that doesn't really have a text representation
< meshcollider>
sipa: to cache them, would you expand() them with the cache function during the existin topup function
< sipa>
meshcollider: right, exactly
< provoostenator>
Why wouldn't it have a text representation?
< sipa>
provoostenator: well the text representation is the entire set of keys, pubkeys, scripts, hdpaths, ... that are currently stored in the wallet :)
< sipa>
i guess you could dump it in hex or something
< provoostenator>
A migration wizard should be able to, at least for standard wallets, turn that into a series of regular descriptors no?
< sipa>
that may be possible, but i don't think that's the priority now
< meshcollider>
It'd be quicker and safer to just move-only the code type of thing into legacy functions
< sipa>
(because then you have to worry about all existing RPCs, and their effect on those descriptors)
< provoostenator>
Right, just depends on what's easier in practice. Personally I suspect it'll be easier to make the wallet _only_ have descriptors, just from a writing tests point of view.
< sipa>
i think it's not too much work to just have a legacy subsystem, and a new system - and a wallet can contain just one of them, or both
< bitcoin-git>
bitcoin/master c84c2b8 practicalswift: tests: Test for expected return values when calling functions returning a success code
< bitcoin-git>
bitcoin/master 9133227 MarcoFalke: Merge #14935: tests: Test for expected return values when calling functions returning a success code...
< provoostenator>
But the RPC thing could be a pain yes.
< sipa>
maybe we even want to forbid mixing them in one wallet
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #14935: tests: Test for expected return values when calling functions returning a success code (master...test-return-values) https://github.com/bitcoin/bitcoin/pull/14935
< sipa>
but forced migration may getting it accepted much harder
< sipa>
so my idea is that every record is a descriptor + some metadata (like gap limit, whether it's change or no), plus some cached keys... and there can be a special "legacy" record that's just all the existing keypool/ismine logic
< phantomcircuit>
the rw config is mostly because the qt stuff writes to random places for settings right?
< provoostenator>
So then you might end up with two seperate wallet systems and a migration tool, where the old system only gets maintenance updates to be able to read from it.
< luke-jr>
phantomcircuit: random? not really; just different from bitcoind
< sipa>
provoostenator: maybe
< phantomcircuit>
luke-jr, i mean it writes to reg on windows
< phantomcircuit>
which is super annoying to change
< sipa>
phantomcircuit: please stick to topic
< meshcollider>
phantomcircuit: we are in a wallet meeting btw :)
< meshcollider>
We could open an issue to discuss the alternative approaches, or just discuss which is easier as we start actually writing the code
< provoostenator>
A new wallet subsystem might also let us cleanly long-term deprecate some wallet RPC methods and replace them with clean ones, that happen to support descriptors?
< sipa>
provoostenator: yup
< provoostenator>
And maybe move to Sqlite3 at the same time.
< sipa>
i think that's completely orthogonal
< provoostenator>
Could be, yes.
< luke-jr>
phantomcircuit: I answered in #bitcoin fyi
< provoostenator>
Anyway, we have some next actions now to continue progress, maybe next topic?
< meshcollider>
#topic progress towards hardware wallets (provoostenator)
< provoostenator>
I'll do some cleaning up of my ugly string concatenation descriptor code after rebase.
< provoostenator>
People can already test it though. It compiles and actually works (at your own risk, so use testnet).
< meshcollider>
jnewbery: looks like it?
< provoostenator>
As in, you can create a new wallet, put read-only keys in it, show address on the device and spend coins. Using achow101's HWI library to talk to the device.
< provoostenator>
So the idea is that users would download that library seperately and just launch bitcoind -signer=../HWI/hwi.py (or some other tool). That way we don't ahve to review individual hardware wallet code.
< meshcollider>
provoostenator: cool \o/
< provoostenator>
With very little code changes this could also work against a gRPC server, but there's some security trade-offs compared to calling commands. We talked about that a few weeks ago.
< provoostenator>
I am trying to keep it generic enough to keep that possible, so e.g. the -signer= command could later also be a URL. But for now, it just executes a command and parses the JSON that command spits out to stdout.
< provoostenator>
Next step for me is to work on GUI support for this. But there's already a pile of prerequisite stuff to review, so don't worry too much about that :-)
< provoostenator>
I personally just like to see the big picture in action.
< meshcollider>
Yes let's not stack too many PRs at once again ;)
< provoostenator>
For GUI proof of concept I'd just like to nag promag about #13100, which we want anyway.
< provoostenator>
By the way, it would be nice if bitcoincore.org and such sites could link to the IRC logs. Many places now point to BotBot and Google can't find these other URLs either.
< provoostenator>
jnewbery: splendit!
< jnewbery>
I expect some/most people won't be around for the next scheduled one (Dec 28th)
< provoostenator>
35C3
< jnewbery>
I almost certainly won't be online for it
< provoostenator>
Maybe bump that, and thus the whole schedule, by one week?
< luke-jr>
might make sense to cancel meetings during Christmas
< sipa>
provoostenator: will you be at 35c3?
< jnewbery>
provoostenator: aj was looking at uploading the IRC logs onto bitcoincore.org every week. Not sure what the progress is on that.
< provoostenator>
That also moves closer to new moon.
< provoostenator>
sipa: I can neither confirm nor deny that :-)
< sipa>
(i've restarted it 3 times, same error each time)
< sipa>
gmaxwell: using sha256d4 (or even simple sha256) in the rng code would require the sha256 to auto-initialize on first use rather than using a dedicated initialize function
< sipa>
that doesn't seem too hard (c++11 has std::call_once for exactly that purpose), but maybe more than we want in one PR
< gmaxwell>
oh damn, we need some init crap to sniff hardware support.
< gmaxwell>
sipa: Well it could directly call the C version if not initilized.
< sipa>
it's not hard to do
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #14963: doc: Add comment to cs_main and mempool::cs (master...Mf1812-docValLocks) https://github.com/bitcoin/bitcoin/pull/14963
< sipa>
std::call_once seems to take 2ns here
< sipa>
that seems acceptable as an overhead to the SHA256 functions (it's equivalent to adding less than a byte of input on average)
< gmaxwell>
well if we care about 2ns we could make the rng state hasher sha256, reduce the chained state size a little and get that time back, ...
< sipa>
no, i don't mean for the RNG
< sipa>
this would be overhead added to every invocation of SHA256 functions
< gmaxwell>
oh. I was thinking you'd just put the call once in the rng functions ahead of the sha256d64 use.
< gmaxwell>
but right okay, only half paying attention.
< sipa>
that would... probably work
< sipa>
an alternative is making sure no global constructors use the RNG
< sipa>
that would have no overhead at all, but is pretty inconvenient for some things
< gmaxwell>
it's also unfortunate that we couldn't make that compile-time fail.
< sipa>
yes
< sipa>
for the RNG itself, currently it's SHA512(new_entropy || previous_state), which is split into rng_output and next_state
< sipa>
which means it's reducing to 256 bits of entropy after every extraction, for no go reason
< gmaxwell>
well I like not keeping around the randomness we just output, it's a liability in memory.
< sipa>
obviously
< gmaxwell>
it seems kinda odd to me that we could end up with with two strenghteners going at once, but I think completely harmless.
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #14964: test: Fix race in mempool_accept (master...Mf1812-testRaceMempoolAccept) https://github.com/bitcoin/bitcoin/pull/14964