< fanquake>
Going to drop the "Needs Backport" label off the PRs in 13253 so we can get a good idea of what's left.
< bitcoin-git>
[bitcoin] Empact opened pull request #13254: Revert "Merge #12870: make clean removes src/qt/moc_ files" (master...make-clean-qt-moc) https://github.com/bitcoin/bitcoin/pull/13254
< bitcoin-git>
[bitcoin] fanquake opened pull request #13255: trivial: Fixed typos and cleaned up language (master...language-cleanup) https://github.com/bitcoin/bitcoin/pull/13255
< fanquake>
^ Can either be merged quickly or closed for now.
< bitcoin-git>
[bitcoin] fanquake closed pull request #10470: Fix for listsinceblock not filtering conflicted transactions (master...listsinceblock-filter-conflicts) https://github.com/bitcoin/bitcoin/pull/10470
< fanquake>
Added "Up for Grabs" labels to both of those, no updates to either for 6moths and nearly a year.
< kallewoof>
fanquake: Yeah, if no reaction even with feedback I think closing is ok. I don't think we should close PR's just because they lie around though. I mean, I have PRs that I am maintaining, with no feedback, that can lie around for months and months. Doesn't mean they should be closed. :)
< kallewoof>
I do think generally we should say 'Close this?' and let the author have a say before closing, since non-maintainers can't reopen their PRs even if they wanted to.
< kallewoof>
I.e. let the authors have an opportunity to close the PR themselves in case they wanna come back to it when they have more time/energy/inspiration.
< fanquake>
kallewoof sure. I have to run out but will comment when I'm back
< kallewoof>
fanquake: No worries, just idly musing.
< bitcoin-git>
[bitcoin] Empact opened pull request #13257: wallet: Fix ReserveKeyFromKeyPool to always test key classification, even if last in the pool. (master...fix-reserve-key-classification-check) https://github.com/bitcoin/bitcoin/pull/13257
< bitcoin-git>
[bitcoin] Empact closed pull request #13257: wallet: Fix ReserveKeyFromKeyPool to always test key classification, even if last in the pool. (master...fix-reserve-key-classification-check) https://github.com/bitcoin/bitcoin/pull/13257
< bitcoin-git>
[bitcoin] kallewoof opened pull request #13259: validation: add a macro for determining if a block is pruned or not (master...block-pruned-macro) https://github.com/bitcoin/bitcoin/pull/13259
< wumpus>
fanquake: thanks, I'll do some review on backports today
< bitcoin-git>
[bitcoin] jonasschnelli opened pull request #13262: Wallet/RPC: Add listsincetx with a stateless (server-side) long polling option (master...2018/05/listsincetx) https://github.com/bitcoin/bitcoin/pull/13262
< jonasschnelli>
maybe jnewbery can look into 12646?
< wumpus>
anyone want to look int othat? or can we just bump it forward if it's not so importent
< jonasschnelli>
I think its okay to bump this forward (as long as we track it)
< wumpus>
yes I don't mean closing it
< jnewbery>
Yes, I can look at 12646 (next week)
< wumpus>
moved to 0.16.2
< wumpus>
other proposed topics?
< BlueMatt>
trashing github
< wumpus>
#topic Trashing github
< BlueMatt>
so it hasnt been working for like 3 weeks now
< BlueMatt>
and I'd kinda like to have something self-hosted with better review tools anyway, which I know a lot of people wanted
< jonasschnelli>
Should we give it more time?... I'm pretty sure they are aware of it
< sipa>
there was some suggestiin (was it on twitter) to use gitlab
< BlueMatt>
so I figured we should do a "do people think its actually a good idea to switch to something self-hosted" semi-poll
< wumpus>
gitlab seems ok
< BlueMatt>
or we could switch to gitlab
< jonasschnelli>
BlueMatt: what alternatives would you propose?
< jtimon>
I like gitlab
< sdaftuar>
seems to me like it's way harder to get it right hosting ourselves
< BlueMatt>
though gitlab seems to have no better review tools than github
< wumpus>
self-hosted I don't know, who is going to babysit this, monitor it and apply security patches etc?
< sipa>
it would e really cool if all pr comment history was in git too
< BlueMatt>
sdaftuar: oh? I mean I kinda disagree
< sdaftuar>
dude we can't even keep the computers in our office running
< jamesob>
self-hosted is very risky and potentially time-consumptive IMO
< BlueMatt>
sipa: does gitlab do that?
< BlueMatt>
sdaftuar: bitcoincore.org does just fine....
< sipa>
BlueMatt: i have no clue
< wumpus>
if no one is, it's going to become worse, not better, at least Github has a dedicated paid team
< sdaftuar>
definitely agree with wumpus
< cfields>
general nack, self-hosting issues aside, Github's network effect is too strong imo.
< sipa>
blockstream uses gitlab internally, which seems to work fine (pribably due to people maintaining it)
< BlueMatt>
i mean we could do self-hosted gitlab
< MarcoFalke>
what advantage does that give, BlueMatt?
< jtimon>
I assume the goal is less unicorns?
< jnewbery>
cfields: +1
< wumpus>
someone from github promised to look into the unicorn issue, maybe we should give them some more time
< MarcoFalke>
gitlab also does hostign for free
< jimpo>
Cursory internet search turned up reviewable.io, which is like a hosted layer on top of GitHub
< sipa>
jnewbery, cfields: what do you suggest instead? waiting until github fixes it?
< jimpo>
free for public repos
< BlueMatt>
MarcoFalke: we can do our own security additions like putting the pr and comment history in git
< cfields>
I can't be the only one who gets irrationally frustrated when the code I want to mess with is on BitBucket..
< BlueMatt>
and have stuff that verifies it
< sipa>
cfields: we'd mirror on github of course
< jnewbery>
In the absence of something better, we should continue nagging them
< wumpus>
cfields: yes, only players like freedesktop can really afford to host on separate infrastructure, for smaller projects the lack of network effect (and having to register separately) is bad
< cfields>
sipa: all things considered, yes, I'd say waiting it out makes the most sense.
< BlueMatt>
jnewbery: I think we *do* have ideas for better things
< cfields>
I think I'm in the minority there, though :)
< jonasschnelli>
Moving away from GitHub seems meh,... especially for a self-hostes solutions... looks after centralizing development
< jimpo>
Who has reached out to GitHub and through what channel so far?
< BlueMatt>
the self-hosting question is more a "will it be maintained" question
< BlueMatt>
not "will it be better"
< jtimon>
I'm ok with both people working on a gitlab instance and people nagging github devs
< sipa>
cfields: i'moretty unconfortable with the fact that network effect is making us stick with a specific provider, even in the oresence of obvious issies
< sipa>
of course, it's not like we could migrate quickly anyway
< wumpus>
'will it be maintained' is really important though to not end up blaming each other
< jamesob>
how much effort will, e.g., DoS protection be for something self-hosted?
< wumpus>
at least now we can blame github people :)
< sipa>
haha
< jtimon>
we could perhaps save money on travis workers by using gitlab-CI too?
< wumpus>
jamesob: exactly...
< BlueMatt>
jamesob: dos protection is 6$/month
< BlueMatt>
and works perfectly
< cfields>
sipa: it's worth considering that the 0.16.1 issues might've never been reported if not for Github's issues
< jnewbery>
BlueMatt: Yes, we have ideas, but that's different from something that's actually running. I don't have any interest in maintaining a self-hosted solution, and I don't think it's worth anyone else's time doing it either
< sipa>
cfields: that's a good point
< wumpus>
we could still use github for *issues*
< wumpus>
gitlab would be for patches and review
< jtimon>
jnewbery: yeah, I'm personally not interested in maintaining it either
< wumpus>
doesn't necessarily need to be the same place
< sipa>
let's move back to sourceforge
< wumpus>
yes tbh I don't think we should change the issue reporting URL
< BlueMatt>
I'm not a fan of using issues and patches/review being in separate places
< wumpus>
I've been spamming that to so many people over the years
< jonasschnelli>
if the unicorns is the showstopper, then better mirror github PR with comments > 20 via API with comment through API function
< jtimon>
or just everything on gitlab, but since we have the github mirror, we will see issues created there by people who missed that the project moved
< wumpus>
I don't really want to move it anywhere else. THe unicorns are only an issue for code review.
< BlueMatt>
jamesob: bitcoincore.org does not use cloudflare (and costs 6$/month), cloudflare sucks ass
< jnewbery>
I also think that the network effect thing is important. What percentage of new contributors/issue reports would we lose if we weren't on github?
< BlueMatt>
jamesob: (and that's for redundant providers)
< BlueMatt>
jonasschnelli: I'd actually be much happier with github if we had a client-side api-based cli-only github interface
< BlueMatt>
(that verified eg pgp signatures on comments)
< wumpus>
there is a github cli interface
< jtimon>
wumpus: well the main point of using github is for code review, no?
< kanzure>
email seems to work for long reviews (diffs)
< jonasschnelli>
BlueMatt: that seems easyer then installing a gitlab solution on a custom server
< jonasschnelli>
*easier
< BlueMatt>
jonasschnelli: its the difference between building a whole app and installing one
< BlueMatt>
so...no, not really
< jimpo>
jnewberry: Who has reached out to GitHub and through what channel so far?
< moneyball>
I am happy to follow-up with GitHub to try and accelerate a fix. Can someone provide me background info on the existing communication we have with GitHub?
< jnewbery>
I've contacted Github support. I don't know who else has
< jtimon>
I think someone reached to them on twitter too
< wumpus>
I have contacted them through tthe contact site, and was told by support that many others have
< moneyball>
Is there an open issue # that I can reference?
< jnewbery>
jtimon: that was me
< jimpo>
I can try asking someone I know that used to work there supporting open source projects
< jnewbery>
moneyball: I'll forward you the email thread
< BlueMatt>
moneyball: a few folks have emailed support@github, which historically has always gotten a response, some other projects were posting responses they got where they were saying "we dont actually know what change we made that triggered these issues, hold on"
< BlueMatt>
buts its been like 3 weeks ago
< moneyball>
jnewbery ok great i'll use that as context and follow-up
< moneyball>
jimpo we can coordinate our efforts if you'd like
< jimpo>
yeah, we can chat about it outside the meeting
< BlueMatt>
ok, so it sounds like consensus is "stick with the broken shit we have now" :/
< BlueMatt>
or at least no consensus on moving to something else
< wumpus>
feel fre to set up something better and convince us to use it
< sipa>
yeah i believe it will require someone to set up a demo
< wumpus>
if not, this is just empty talk, we *have* nothing better
< wumpus>
any other topics?
< MarcoFalke>
And a plan to transition all open patches to the new review system?
< sipa>
right; the question is whether we should look into alternatives
< sipa>
not so much whether we should or shouldn't move
< wumpus>
right
< cfields>
iirc some alternatives support oath login via github
< BlueMatt>
yea, it seems like lack of consensus to move even if we find something good
< cfields>
that would go a long way towards shutting me up
< BlueMatt>
which was mostly why I brought it up
< wumpus>
I'm open to being convinced to use something else, if it's really better
< jimpo>
cfields: I agree a code review tool with GitHub integration (where main repo is still hosted there) is ideal
< jnewbery>
BlueMatt: If there was something else better running now AND there was a way to migrate all state AND we wouldn't lose contributors by switching AND we have someone committed to maintaining it, then I'd be open to it. Without that, I think it's a non-starter.
< jonasschnelli>
Moving away from Github just because of load issues for three weeks seems insane...
< BlueMatt>
jtimon: it sounds like definite "no", so I'm not gonna spend time looking into it
< jtimon>
in the meantime, prs with many comments get the unicorn and we have to try again until it loads
< wumpus>
#topic separate wallet from node (jnewbery)
< BlueMatt>
jonasschnelli: there are way more reasons people dont like github
< sipa>
jonasschnelli: not being able to move away from github just because of network effect seems scary...
< jamesob>
the upside is that this is an additional incentive to keep PRs small :)
< jtimon>
BlueMatt: fair enough
< cfields>
jonasschnelli: I agree. But I think the frustration comes from the helplessness that it's brought to light.
< wumpus>
this discussion is starting to repeat itself
< jonasschnelli>
sipa: that is a point we should take into consideration,.. but stop focusing on load issues
< sipa>
ok, next topic it seems
< BlueMatt>
oh, I have 2 more topics....
< wumpus>
I don't think there's realistically any chance of anything replacing github until someone sets up a feasible alternative and shows us that it is better
< sipa>
jnewbery: your topic
< jonasschnelli>
(#topic separate wallet from node (jnewbery))
< BlueMatt>
sipa: well I'm gonna look into having a cli app that checks signatures off github api comments/etc
< jnewbery>
#10973 is a big PR, but I think it's very worthwhile
< BlueMatt>
cause I think that would alleviate a lot of it
< jnewbery>
jamesob and I have both reviewed now, but it requires continual rebase
< jnewbery>
There are a lot of PRs competing for high priority for review, but I think it'd be great to make some progress on this one
< promag>
place in high priority this week?
< jamesob>
I'll start a round of manual testing if we can get another reviewer or two
< jimpo>
can it be broken up at all?
< jnewbery>
so, next steps would be: concecpt ACK/NACKs
< BlueMatt>
sounds like a high-priority-for-review nomination?
< jnewbery>
and if people think it's too much to review in one go, ryanofsky is happy to split
< jimpo>
1500+ lines is too big IMO
< wumpus>
oh no, not more high priority for review
< jonasschnelli>
jnewbery: this is orthogonal of using light client mode to decouple the wallet from the node? right,... it's more architectural?
< jnewbery>
jimpo: it's mostly very mechanical changes, but yes it's a large +-loc
< wumpus>
is it blocking anything? is it importnat for 0.17?
< jnewbery>
there are only a couple of commits that require deep review
< sipa>
jnewbery: thanks for bringing it up; big PRs are sometimes unnecessarily scary to dig into
< jnewbery>
wumpus: it blocks (but doesn't necessarily have to lead to) process separation
< jonasschnelli>
But I guess it's hard/impossible to break it into smaller PRs
< jamesob>
jonasschnelli: it makes explicit the bitcoind interface that the wallet relies on, which is in itself useful but also if we want to do any kind of process separation
< jnewbery>
jonasschnelli: you're correct
< wumpus>
process separation is not something we'll have for 0.17 anyway
< jnewbery>
so at this stage it's more of a concept review beg from me, and a poll on whether russ should spend time splitting it up
< jimpo>
I'll give it a pass by tomorrow
< jnewbery>
The reason I raised it is that because of the frequent rebases, reviews go stale very quickly, and it's now had two ACKs
< jnewbery>
thanks jimpo
< ryanofsky>
i actually don't think it's hard to split up, first 6 commits seem to group together nicely, with rest of changes more independent
< jnewbery>
that's all from me. If 2 or 3 regular reviewers are happy to concept review, I think that's good progress
< jonasschnelli>
If we assume the long term goal is process separation (where the wallet will turn into a pure transaction-filtering-thinkg), isn't it, that the coin-selection & signing elements in this interface will get throw away later?
< BlueMatt>
topic: unverified-block-message
< BlueMatt>
topic: queue drain lock assertions to avoid deadlocks
< ryanofsky>
jonasschnelli, there aren't any coin-selection or signing things in node/wallet interface in that pr
< jonasschnelli>
maybe I should review the PR first...
< sipa>
BlueMatt: your topic
< BlueMatt>
so sdaftuar pointed out an old issue that we never really addressed where if you relay someone a script-invalid block they may announce it to their peers via compact blocks high-bandwidth-mode and then if you for some reason fall back to downloading it via getdata due to short id collision or so (we dont think there is a way to game it), then you'll end up disconnecting that peer for never responding to your request
< jtimon>
re 10973 I agree I would preffer a few smaller ones, specially if there's commits that needs deeper review
< BlueMatt>
we only came up with two real potential solutions: a "no, I'm not gonna send you that block" message (ie a not-batshit-insane reject message) or a "here is a block that may be invalid, but is valid pow+merkle root ala compact blocks requirement" message
< BlueMatt>
or, I guess the second one is a getdata type
< sipa>
BlueMatt: we have "notfound" also
< BlueMatt>
sipa: isnt that a reject type?
< sipa>
no
< wumpus>
NetMsgType::NOTFOUND
< jnewbery>
*5 chaincoders furiously grep for notfound*
< sipa>
it's just a "i can't give you these INVs"
< sdaftuar>
oh wow
< BlueMatt>
sipa: ah, but we dont use it for blocks
< BlueMatt>
only txn
< sipa>
ah
< BlueMatt>
still, easier would be a "here is a block that may be invalid" as that would remove the ABC in ProcessGetBlockData
< wumpus>
the client ignores it just the same though
< cfields>
BlueMatt: so, lots of NOTFOUNDs :)
< sipa>
BlueMatt: we should have had that before compact blocks, i guess
< BlueMatt>
sipa: should have had what?
< BlueMatt>
sdaftuar: points out that it can be wholly independant, just a new getdata type
< sipa>
a possibly invalid block relay
< sdaftuar>
i think we could just add a new BLOKC response type
< sdaftuar>
BLOCK_COULDBEBAD
< BlueMatt>
well or both or whatever
< sipa>
0xDEADB10C
< sdaftuar>
where if someone requests a block but you don't know if its valid, or you know it's invalid, you return a different message containing the block to indicate that
< wumpus>
hehe
< BlueMatt>
yea, so old nodes would ignore it, and you'd just be sending a new message type
< sdaftuar>
currently we would let the peer time us out instead
< sipa>
sdaftuar: but only if you know they support such a message
< BlueMatt>
or not
< sdaftuar>
meh, sure, but not even needed imo
< BlueMatt>
doesnt really matter
< BlueMatt>
I mean they're about to disconnect you either way
< sipa>
fair
< sipa>
that makes compatibility really easy
< sipa>
i guess... suggest something and write a bip?
< BlueMatt>
oh, wait, no, you also want a new getdata type
< BlueMatt>
cause otherwise you still need the ActivateBestChain in ProcessGetBlockData
< BlueMatt>
which would require negotiation
< sdaftuar>
yeah ok
< BlueMatt>
so either that, or we start using NOTFOUNDs, I guess
< BlueMatt>
I'm not sure what I prefer, so.....thoughts?
< sdaftuar>
i think notfounds are worse because of the case where the block might not have been validated either way
< sdaftuar>
it complicates download logic
< wumpus>
if there is no specific reason to re-use NOTFOUND, a new message is much better imo
< BlueMatt>
yea, its not really "notfound" its "I may find this soon"
< BlueMatt>
or, really, "I dont currently find this"
< sipa>
i don't think we should do protocol design in this meeting
< BlueMatt>
well, there's a few options, so...good to ask
< wumpus>
not the design but discussing it as a concern is valid
< wumpus>
agree BlueMatt
< BlueMatt>
obviously requires BIP and whatever else
< cfields>
my only request (same as always) is avoiding "if(on_thread1) foo() else bar()" type logic
< BlueMatt>
cfields: nah, more a DUMMY_LOCK(on_thread1); AssertLockNotHeld(on_thread1) :p
< promag>
jonasschnelli: will fix
< jonasschnelli>
thx
< promag>
np, see you later
< bitcoin-git>
[bitcoin] laanwj opened pull request #13265: wallet: Exit SyncMetaData if there are no transactions to sync (master...2018_05_wallet_syncmetadata_empty_range) https://github.com/bitcoin/bitcoin/pull/13265
< gmaxwell>
cfields: I dunno if you discussed this with pieter before, but an alternative to your UHS that I suggested to him previously was that if you have a per-node secret salt S, then for all "hash-like" scriptpubkey types, you could store as a key H(s || script-type || txid || vout || scriptpubkey-data)[0:16] and value is just the compact_amount. This would allow storing almost all utxo in about 20
< gmaxwell>
bytes, have a reasonable security argument, and require no network communications overhead.
< achow101>
damn, I forgot there was a meeting today
< achow101>
wumpus: can I get #12136 for high prio?
< gmaxwell>
The security argument is that with an attacker unknown salt S, the only property you need from the hash resistance to chance collisions. (also even if there were one, it would just break a single node).
< gmaxwell>
cfields: if it isn't obvious how you verify against it, when a transaction comes in, you take its input txin and vout indexes, and look at the scriptsig and construct the appropriate scriptpubkey from it. Then check if thats in the database.
< jamesob>
*low, ugh
< cfields>
gmaxwell: yea, we discussed that as well. I'm not opposed, but I wanted to introduced the idea as a near drop-in for the status-quo, security wise. That salt ends up acting as a private key somewhat, which is not major but not trivial either
< cfields>
gmaxwell: please mention that on the thread though. I just wanted to introduce the paradigm, I'm sure there are lots of better ways to execute it :)