< gmaxwell>
sdaftuar: do we learn about a peer's tip from GETHEADERS messages they send us... e.g. if they send us a getheaders whos locator starts with our current tip will we update them to that
< sdaftuar>
gmaxwell: my understanding of the scheduler is that it'll start scheduling callbacks at some point after startup, so i wouldn't expect the network to be completely synced
< sdaftuar>
gmaxwell: also there's random drift, since the scheduler schedules the next callback N milliseconds after the prior one finishes
< sdaftuar>
but worth discussing whether the interval is short enough that there still might be too much concentration of everyone trying to find a new peer
< sdaftuar>
gmaxwell: regarding getheaders/locator -- no we don't update pindexBestKnownBlock from a peer's locator
< sdaftuar>
we use pindexBestKnownBlock to figure out what blocks we can download from our peer
< sdaftuar>
and the peer will update its locator based on headers it knows (not blocks!) during initial headers sync
< sdaftuar>
in order to sync the whole headers chain (and given the MAX_HEADERS_RESULTS constraint on headers messages)
< bitcoin-git>
[bitcoin] sdaftuar closed pull request #11534: Evict outbound peers if tip is stale (master...2017-10-stale-tip-eviction) https://github.com/bitcoin/bitcoin/pull/11534
< achow101>
meeting?
< sdaftuar>
meeting
< promag>
+1
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Oct 26 19:00:53 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< cfields>
11490 looked good last i looked, will take a quick look at the last changes and re-ack
< gmaxwell>
sdaftuar: I already ACKed it. I think it's pretty great.
< sdaftuar>
thanks!
< achow101>
gmaxwell: found a peer that his node with 11490 kicked. we're not sure whether it was kicked for being brain dead or spynode
< achow101>
or ibd
< gmaxwell>
but is should have been kicked regardless.
< wumpus>
yes was about to say that
< wumpus>
seems an improvement in any case to kick it :)
< jtimon>
I was going to say if there was anything stopping 8994, but it needs rebase...
< achow101>
ok, I just wasn't sure that it would effect nodes in inbd
< gmaxwell>
Yes, I've had a node running the patch for a few days and after the first day I started cycling out all my outbounds every few hours with the hopes of hitting some broken peers.
< gmaxwell>
achow101: yes, it should-- they're useless to us as outbound targets.
< achow101>
oh right, duh. outbound..
< gmaxwell>
.yes, this is outbound only
< gmaxwell>
and it avoids acting on 4 peers. so even if it goes nuts and kicks things it shouldn't the damage is limited.
< bitcoin-git>
[bitcoin] sdaftuar opened pull request #11568: Disconnect outbound peers on invalid chains (master...2017-10-disconnect-outbound-peers-on-invalid-chains) https://github.com/bitcoin/bitcoin/pull/11568
< sdaftuar>
also i think i have a fix to #11446 that should satisfy concerns raised in that PR ^^^
< cfields>
tangentially, does it not make sense to take their blockheight in version into consideration? sure it can be spoofed higher, but if it's low and we're in IBD (and presumably they are too), is it worth it to hang around?
< gmaxwell>
to reiterite why this specific patch is important; beyond general braindead peer elimination, it addresses the case where you have peers on incompatible consensus rules which you aren't discovering because their chain has less work, so you aren't fetching their invalid blocks.
< sdaftuar>
it's a refactor unfortunately, so not sure it is a good candidate for backport to 0.15. but it was simple to do
< gmaxwell>
cfields: well someone could have a lower height but more work chain
< wumpus>
yes, so for 0.15.0.2 we have open at the moment, apart from the one just discussed: #11560 #11446 #10593
< meshcollider>
Is #11531 also aimed for 0.15.0.2?
< gribble>
https://github.com/bitcoin/bitcoin/issues/11531 | Check that new headers are not a descendant of an invalid block (more effeciently) by TheBlueMatt · Pull Request #11531 · bitcoin/bitcoin · GitHub
< jnewbery>
#10593 seems to be reverting unrelated code with no rationale, so I think that can be untagged
< achow101>
I guess 11446 has some edge cases that could be problematic
< gmaxwell>
for some reason I thought what we were doing in 11446 was outbound only. I agree that our agressiveness increases here should be outbound only.
< luke-jr>
jnewbery: reverting what? it removes tests that check for the behaviour that is being removed
< wumpus>
ok added 11531
< gmaxwell>
it's important that we don't fully partition old nodes during a softfork.
< achow101>
it would be better if we could get more granular errors from processnewblockheaders
< wumpus>
how much time do we have left?
< sdaftuar>
11568 is the same as 11446, except it avoids disconnecting hb compact block peers, and only applies to outbound peers
< wumpus>
sdaftuar: ok we should remove one, then :)
< sdaftuar>
agreed :)
< wumpus>
eh wait 11568 isn't tagged 0.15.0.2 at all
< cfields>
heh, it's getting hard to keep up with these
< achow101>
wumpus: it was just made
< sdaftuar>
i was trying to open it before the meeting started, doh
< wumpus>
cfields: very hard, yes
< achow101>
if 11568 is 11446 but only for outbound peers, I'm fine with that
< jnewbery>
luke-jr: I haven't fully dug into it, but it regresses #8305, no?
< BlueMatt>
I mean if we want a 0.15.0.2 we literally need to rc tomorrow or this weekend, imo
< gmaxwell>
11568 looks good on first run through.
< cfields>
disclosure there: I'll be away after tomorrow night until tuesday morning
< wumpus>
BlueMatt: what can we realistically merge before that time?
< luke-jr>
jnewbery: I don't think so - 8305 disconnects under certain conditions, but 10593 disconnects a superset of those conditions
< cfields>
i can make plans to be available for building if really necessary
< BlueMatt>
wumpus: I dont think a sufficient set to make 0.15.0.2 worth it
< gmaxwell>
I think probably if you try you can merge one PR per minute... we could empty out github by then, no problem.. just lots of button pushing!
< meshcollider>
lol
< wumpus>
well we have some fixes on the 0.15 branch already that are nice
< BlueMatt>
heh
< wumpus>
but I agree it kind of misses the point
< BlueMatt>
wumpus: anything worth an 0.15.0.2?
< wumpus>
BlueMatt: well it's a minor-minor release, that's not a high bar, but yes
< BlueMatt>
I mean things like #11531, which may be important as far as our b2x-shitshow fixes go, are smack-dab in the middle of consensus code and have not review yet?
< gribble>
https://github.com/bitcoin/bitcoin/issues/11531 | Check that new headers are not a descendant of an invalid block (more effeciently) by TheBlueMatt · Pull Request #11531 · bitcoin/bitcoin · GitHub
< wumpus>
BlueMatt: nothing that warrants hurring it though
< BlueMatt>
I suppose it may be worth it to get an 0.15.0.2 out like day-before b2x-shitshow
< gmaxwell>
even a small portion of the network running it is protective.
< BlueMatt>
then at least if some asshat spins up a billion stupid sybil nodes there is a response
< morcos>
BlueMatt: +1
< BlueMatt>
but we need to be clear what our target is here
< gmaxwell>
at least if we cover the major possible cases: Fork has low to no hashpower, fork has higher hashpower for the case where we're completely partitioned by surrounded by forknodes or where we're partially surrounded.
< BlueMatt>
and really focus on it
< BlueMatt>
there hasnt been much progress the last 3 weeks
< BlueMatt>
(I'm to blame there, too, I've been mostly mia)
< gmaxwell>
I think there has been a lot of progress.
< gmaxwell>
sdaftuar has been right on top of making changes.
< BlueMatt>
sorry, lots of progress on making new prs, rewriting prs, talking about issues, lack of *merge* progress
< wumpus>
yes there absolutely has been a lot of progress, PR after PR
< BlueMatt>
or finalizing review for things to get merged
< wumpus>
but very little merging, yeah
< morcos>
have we finalized the list
< morcos>
lets focus on that
< morcos>
and then everyone commit to reviewing 2 of them over the next 24 hours
< BlueMatt>
well one thing on it was opened today cause it was decided to also be important/replace other things
< morcos>
and we'll see where we stand
< wumpus>
no, it seems to change every week
< morcos>
wumpus: exactly
< BlueMatt>
but, yea, I think if we want to see hhis happen, we can get it out day-or-two-before-ish, but we need review *today*
< cfields>
agree with gmaxwell about focusing on those things that may actually be of significance in the next month
< wumpus>
as cfieds said it's hard to keep track of
< BlueMatt>
I think they're all tagged now, no?
< gmaxwell>
To get the full coverage of those sets of cases, we need 11490, 11560, and 11568-or-11446
< morcos>
focus... BlueMatt, sipa, gmaxwell, sdaftuar you guys have been thinking abou tthis the most... look at the list right now and be confident it is right and if not fix it
< BlueMatt>
yea, 10593 got nack'ed (correctly, imo) a while ago
< gmaxwell>
11490 covers the case where fork has lower hashpower and doesn't completely surround you, 11560 covers where it has higher hashpower, 11568-or-11446 covers where it has lower and may completely surround you.
< luke-jr>
does something else cover the cases 10593 does?
< rafalcpp>
sorry if stupid question, but re `semOutbound = new CSemaphore(std::min((nMaxOutbound + nMaxFeeler + nMaxExtraOutbound), nMaxConnections));` if we're at nMaxOutbound == nMaxConnections doesn't it mean node in such conditin will not try to resolve being stalled? dunno if that's any issue
< morcos>
luke-jr: explicit case you're worried about not being covered (soft forks? , we can worry abou tthose later)
< BlueMatt>
luke-jr: tbh, I dont actually have any fucking clue what that pr is *supposed* to do in the context on b2x-shitshow
< achow101>
rafalcpp: not now
< sdaftuar>
rafalcpp: let's discuss after (thanks for taking a look!)
< BlueMatt>
rafalcpp: sorry, mid-meeting atm
< morcos>
rafalcpp: hi
< gmaxwell>
achow101: that was related to the PR that adds an extra connection
< luke-jr>
morcos: Bitcoin is almost a softfork relative to 2X
< luke-jr>
BlueMatt: it is necessary for people to switch from 2X to Bitcoin, or run them both
< IniGit>
hello
< BlueMatt>
luke-jr: you cannot switch back and forth, your blockindex will be corrupt (yay shitty fork developers)
< IniGit>
I read the whitepaper of ethereum and I have a question (it is the same for bitcoin):
< IniGit>
APPLY(S,TX) -> S'
< IniGit>
My question is since S is defined as a set of UTXO, but the blockchain does not store each balance in a block, is this set of UTXO only preset in RAM and not on the blockchain. Is it calculated by the node by going thhrough each block since the genesis block and only present in RAM?
< luke-jr>
BlueMatt: different datadirs..
< luke-jr>
or even different machines
< wumpus>
IniGit: not here, not now
< morcos>
luke-jr has a bit of a point there
< gmaxwell>
luke-jr: it's unclear of specifically what you're turned about, don't get into an argument in the weeds, state what the overall concern is.
< IniGit>
where can I ask this question and get more knowledge?
< gmaxwell>
IniGit: #bitcoin
< sdaftuar>
are we talking about relaxing bans to be disconnects instead? i generally agree with that
< BlueMatt>
luke-jr: tbh, so what? our banning is deliberately slow, if you get banned from some small percentage of the network, slowly, over time, for running 2x, sucks for you
< morcos>
gmaxwell: if someone on your IP runs a 2X node, your IP gets banned, and you cna't then run or simulataneiously run a core node
< luke-jr>
gmaxwell: right now, we can peers that send invalid blocks, which means their Bitcoin nodes will get rejected from the p2p network too
< BlueMatt>
(like one peer per block kinda slow)
< achow101>
gmaxwell: I think the concern is if someone runs 2x and gets themselves banned, if they switch back to bitcoin they can't connect to the network anymore
< luke-jr>
ban*
< gmaxwell>
morcos: nothign we can do about that now regardless; as it's a property of the widely deployed network.
< wumpus>
achow101: only if *everyone* banned them, which is unlikely as hell!
< wumpus>
achow101: if a few nodes they were connected to banned them they will certainly find others
< luke-jr>
wumpus: when 2X gets banned from Peer A, it will move on to Peer B, etc
< morcos>
gmaxwell: hmm.. ok fair, and it'll take a lot of blocks to get banned by most of the network...
< gmaxwell>
luke-jr: yes, but this takes longer than a day to cycle through all reachable nodes that way.
< luke-jr>
gmaxwell: so long as not all peers ban, they will eventually find ones with the newer code..
< morcos>
luke-jr's pull should be maybe considered for soon release, in case there is extended period of two chains
< karelb>
sorry for breaking in, the plan is that you cannot simultaneously run 2x and btc on the same IP? that is unfortunate
< morcos>
but maybe 0.15.1 espeically if its not ready yet
< wumpus>
gmaxwell: and by that time the first ones will have unbanned them again
< luke-jr>
gmaxwell: even with the peers banning them?
< morcos>
karelb: that's not the plan, thats an unfortunate side effect of the existing software
< gmaxwell>
luke-jr: yes, because the banning goes slowly.
< gmaxwell>
karelb: 2x foolishly connects to non-2x nodes and will relay them invalid blocks, so it will get itself banned.
< achow101>
banning also requires blocks to be found, so ...
< achow101>
that means 144 peers to switch through per day, on average
< luke-jr>
gmaxwell: just because it hits 20% DoS penalty each header?
< wumpus>
karelb: it could have been resolved with e.g. service bits if 2x was willing, but they're insisting on making this a mess, so we have to do the least harmful thing for the existing bitcoin network
< karelb>
oh OK. That happens with the current core node
< BlueMatt>
karelb: if you run 2x nodes without their broken -pretendimnot2x then no, you cannot, if you dont use that option, you should be fine
< gmaxwell>
karelb: yes.
< wumpus>
BlueMatt: exactly
< karelb>
BlueMatt 👍 great
< BlueMatt>
(which is another point against 10593)
< gmaxwell>
luke-jr: also as matt points out, if you don't undermine service flag disconnects you won't get banned by bitcoin 0.15+ peers, so I think that answers your concern/.
< luke-jr>
ok
< BlueMatt>
if you run with -imstupidandignorereason, you should get banned, thats your problem, not mine
< karelb>
what would be the reason of running that option? what is the incentive for the 2x node to connect to core nodes?
< gmaxwell>
in the long run I want to move away from banning more or less entirely but that isn't a thing to worry about for now.
< BlueMatt>
karelb: please not mid-meeting
< karelb>
ok
< morcos>
ok yes, thats a good point that we should publish, unfortunately lots of companies will need to run both
< karelb>
sorry
< BlueMatt>
gmaxwell: lots to be fixed in our dos/ban/disconnect logic, indeed
< morcos>
ok back to question... list is good, please ack if you know what you're talking about. alex was here.
< sdaftuar>
getting back to 0.15.0.2 -- i think the currently tagged PRs cover all the cases i think we would ideally cover pre-b2x
< achow101>
list looks good to me
< wumpus>
sdaftuar: great!
< gmaxwell>
What sdaftuar said
< wumpus>
let's focus on getting these reviewed and merged as soon as possible then
< achow101>
+1
< wumpus>
and lot's not add any new ones unless absolutely necessary
< gmaxwell>
unless some interesting bug crops up I don't see why we would.
< meshcollider>
Sounds good
< BlueMatt>
ok, so #action 24-hour review sprint?
< gmaxwell>
like maybe B2X's developer tells us about this mysterious instability in 0.15. :)
< morcos>
i can't wait to tell them about my embargoed accidental hard fork
< BlueMatt>
gmaxwell: lol, uh huhhhhh
< luke-jr>
FTR, the problematic option is -advertise2x=0 or -noadvertise2x
< BlueMatt>
aka -shootmeinthefacekthx
< cfields>
BlueMatt: i'll commit to a 24hr sprint, at least ack/nack/cfields was here on all of those PRs.
< wumpus>
I'll try
< gmaxwell>
I'll test and review all that I haven't yet, of course.
< BlueMatt>
ok, just gotta finish caching debug.log writing on fibre nodes then I'll do it
< gmaxwell>
BlueMatt: we could also release note this point-- that running -shootmeinthefacekthx will cause your IP to get banned by bitcoin nodes when the fork happens and make it harder to switch back or run both.
< gmaxwell>
so uh. I hate to do this.
< gmaxwell>
but
< achow101>
oh no
< gmaxwell>
... I believe a one liner is possible to detect if our chainstate DB has been corrupted by running b2x post fork... would it be worthwhile to have that?
< gmaxwell>
e.g. check out block at the fork height and see if its too big.
< kanzure>
detect and warn?
< wumpus>
yes, that would be worth it
< gmaxwell>
and then shut down with a polite fuck you instead of just sitting stuck.
< kanzure>
detect and exit?
< wumpus>
yes
< BlueMatt>
yea, I think so :'(
< cfields>
gmaxwell: that sounds like exactly the kind of thing we should be aiming for in 0.15.0.2 :(
< gmaxwell>
all we can do is exit and tell you to reindex.
< achow101>
I guess..
< morcos>
in my opinion it'll be more than 24 hours until we agree if a 1-liner makes sense. i think we should just tell people you must reindex chainstate if you switch back...
< jnewbery>
polite fuck you == "please use invalidateblock RPC" ?
< BlueMatt>
i guess at least thats easy to review
< luke-jr>
gmaxwell: why shut down? we can already rewind..
< gmaxwell>
oaky I'm sorry for the scope creep. It should be a near oneliner.
< luke-jr>
jnewbery: need to automatic it, because RPC won't work if we shutdown
< BlueMatt>
jnewbery: no, I think probably easier to just printf("please reindex") exit();
< wumpus>
that's more work
< gmaxwell>
it's not easy to test unfortunately.
< luke-jr>
BlueMatt: we already have code to rewind for segwit rechecking
< wumpus>
for master it probably makes sense to make it automatic
< wumpus>
but that's not realistic for 0.15.0.2
< BlueMatt>
yea, i dont want to test any of this, or think about complicated interactions
< BlueMatt>
just printf and exit
< wumpus>
as gmaxwell says, warn+exit is better than mystieriously getting stuck
< wumpus>
that's the aim here
< achow101>
I support the thing that requires less work to review
< sdaftuar>
i don't know that i think it's worth it, but i don't object i guess if other people want to add this "feature"
< gmaxwell>
okay, I'll do the simplest possible first we could also consider others for master/later/etc...
< BlueMatt>
morcos: points out this does not work on a pruned node, i forgot about that....
< wumpus>
sdaftuar: less mysterious bug reports...
< wumpus>
sdaftuar: that's always a plus
< morcos>
can we agree that this extra PR is lower priority than the others, lets not risk getting 0 of them out
< gmaxwell>
SURE
< wumpus>
morcos: I agree with that too
< gmaxwell>
I don't even 'want' it so much as I realized it was a problem people will encounter which we could address.
< wumpus>
I think it's worth doing in general, don't care if it doesn't get into 0.15.0.2
< morcos>
tag it "Extra Credit"
< sdaftuar>
haha sounds good to me
< wumpus>
fine for 0.15.1 too
< wumpus>
heh
< gmaxwell>
well I'll try something. decide if you want to review it or not.
< kanzure>
might make sense for flip floppers later
< gmaxwell>
after the fork it can just hardcode the hash. :)
< wumpus>
yep
< gmaxwell>
e.g. like doing an invalidateblock <b2xcrap> at startup.
< gmaxwell>
I agree it's certantly less important in part because we can address with announcements/release notes-- don't do this.
< * sipa>
here for a minute
< BlueMatt>
sipa: we're doing 24 hour code review spring for 0.15.0.2, you owe us review on 4 prs today! :p
< sipa>
BlueMatt: i'll try!
< achow101>
not just 4 PRs, the 4 PRs tagged for 0.15.0.2
< luke-jr>
we could make a BLOCK_OPT_WITNESS-like thing and require it for the 2X height block only.. but that'll break normal upgrades too
< bitcoin-git>
bitcoin/master c60fd71 Suhas Daftuar: Disconnecting from bad outbound peers in IBD...
< bitcoin-git>
bitcoin/master 5a6d00c Suhas Daftuar: Permit disconnection of outbound peers on bad/slow chains...
< bitcoin-git>
bitcoin/master e065249 Suhas Daftuar: Add unit test for outbound peer eviction
< achow101>
mmm. subject limes
< cfields>
gmaxwell: it matters who's first though. If only we had some way to trustlessly timestamp things...
< luke-jr>
ICO is just scamcoins 2.0; BlueMatt once had a generator..
< sdaftuar>
wumpus: thanks!
< esotericnonsense>
gmaxwell: well the title isn't wrong.
< sdaftuar>
rebase time...
< rafalcpp>
sdaftuar: right, it's the question that was posted above
< * esotericnonsense>
wanders off to 'make some money' on regtest
< gmaxwell>
cfields: it could be a device that ticks at a regular interval
< bitcoin-git>
[bitcoin] laanwj closed pull request #11490: Disconnect from outbound peers with bad headers chains (master...2017-10-outbound-peers-good-chain) https://github.com/bitcoin/bitcoin/pull/11490
< maaku>
esotericnonsense: don't you go monetizing regtest coins!
< sdaftuar>
nMaxConnections is generally much larger than nMaxOutbound, but yes if someone set nMaxConnections very low, then i think that could cause issues. but that's no different eg than running with -connect= or something, i think?
< sdaftuar>
rafalcpp: ie if you run with non-standard p2p config, then i think you could run into issues... maybe worth release noting?
< cfields>
gmaxwell: 15 seconds? :p
< luke-jr>
btw, the advertise2x warning seems more appropriate for a blog post than release notes, since it has nothing to do with our release
< meshcollider>
Only 3 PRs left to review now :)
< rafalcpp>
sdaftuar: dunno, maybe? :) I didn't yet developed on bitcoin. Wouldn't it be possible to increase both sides of min?
< gmaxwell>
luke-jr: well I dunno the release note is "if you plan on using this program with that program on the same computer"
< rafalcpp>
though then we're skipping check for system limit of connections so maybe not. sdaftuar
< sdaftuar>
rafalcpp: i think it makes more sense for the user to continue to have a toggle for the overall number of connections, and just explain what the consequence is
< luke-jr>
gmaxwell: it's also true for older releases of ours though
< gmaxwell>
if we have a zillion inbound connections we probably don't need the anti-partition rotation anyways.
< wumpus>
meshcollider: review is still welcome after something is merged :)
< notmike>
Its time. I'm ready.
< rafalcpp>
sdaftuar: to me a release note sounds good. unless also logging a warning when it happens, if that code isn't frozen