< cfields>
BlueMatt: I didn't like that change at the time because 1. We weren't always disconnecting (or checking fDisconnect) as necessary at the time, and 2. It changes the "you can only send 1 version message" semantics a bit. 1. should be fixed since fedea8a14. I think your change is probably ok, but I think we should think through 2 a little.
< BlueMatt>
I know we also discussed moving off of the "nVersion is set means connected" to using the fSuccessfullyConnected flag again
< BlueMatt>
they're kinda confused and redundant atm
< cfields>
yea
< cfields>
that would be my preference, only issue there is defining "successfully connected"
< cfields>
BlueMatt: as a weird example, I think your change would allow for sending infinite version messages in which the addrMe portion fails to deserialize
< cfields>
so a peer could change their nVersion a bunch of times before locking it in
< cfields>
(i don't know what good that would do, but we certainly shouldn't be allowing that)
< BlueMatt>
I'd file that under "undefined behavior" I believe in that case we may be required to make a best effort to eat the sender's cat
< BlueMatt>
but short that, I'm not sure if we care that they can do that?
< BlueMatt>
as long as we actually gate on fSuccessfullyConnected
< cfields>
BlueMatt: right, that was my point. atm we don't in many places
< BlueMatt>
cfields: do you have time to look at that, or should I just whip something up with fSuccessfullyConnected after I finish review of one or two others?
< BlueMatt>
well atm we check nVersion != 0 pretty much everywhere, i think
< BlueMatt>
as a proxy for connectedness
< cfields>
BlueMatt: right, and in the above scenario, we don't consider them connected, but their nVersion is set.
< BlueMatt>
ok, so replace nVersion != 0 with fSuccessfullyConnected everywhere, i guess?
< cfields>
i think so.
< BlueMatt>
k
< cfields>
and at this point, we can remove the assert
< BlueMatt>
I'll do it after lunch if you havent gotten to it
< BlueMatt>
I'd prefer leaving it in?
< BlueMatt>
afaict its super easy to fix
< BlueMatt>
I'm incredibly confident in the above change
< BlueMatt>
(as its equivalent to the change I've been running for months now)
< cfields>
BlueMatt: point me at a node running it, let's see if i can bring it down :)
< BlueMatt>
the fibre nodes
< BlueMatt>
:p
< cfields>
heh
< BlueMatt>
I'll swap my public node to master soonish
< BlueMatt>
it gets a shitload of connection churn
< BlueMatt>
and fun spy nodes and shit which do batshit crazy things
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9575: [trivial] Add comment about unreachable code (master...never-executed-comment) https://github.com/bitcoin/bitcoin/pull/9575
< gribble>
https://github.com/bitcoin/bitcoin/issues/9570 | Block Wallet RPCs until wallet is synced to our current chain by TheBlueMatt · Pull Request #9570 · bitcoin/bitcoin · GitHub
< BlueMatt>
sipa_/wumpus: so I think the only way to fix the regression introduced in #7946 without giving up the gains it gave us is to introduce a second cs_wallet - cs_wallet_locked_before_cs_main
< BlueMatt>
specifically, this half-reverts #9570 to get to the behavior intended in #7946 without introducing the getbalance-etc-may-return-data-from-mid-block-processing regression
< gribble>
https://github.com/bitcoin/bitcoin/issues/9570 | Block Wallet RPCs until wallet is synced to our current chain by TheBlueMatt · Pull Request #9570 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #9578: Add missing mempool lock for CalculateMemPoolAncestors (master...2017-01-fix-missing-wallet-mempool-lock) https://github.com/bitcoin/bitcoin/pull/9578
< cfields>
BlueMatt: ok, i've been staring at this for most of the day and i'm reasonably satisfied now. I'm good with your change with 2 tweaks
< cfields>
1. deserialize the whole thing locally. Same change you made for nVersion, let's go ahead and do that for everything. That way we don't end up in a weird state if something throws
< cfields>
and 2. just a quick reordering that moves the assignment into CNode up a bit, and sets nVersion/fSuccessfullyConnected last.