< gmaxwell>
node ID should be monotone and increasing, so it should if anything be a better latest peer criteria than connect time.
< sdaftuar>
gmaxwell: thanks, that sounds better
< sdaftuar>
gmaxwell: i thought of another problem with that patch, that i'm not sure how to best address
< sdaftuar>
it seems suboptimal to disconnect a peer that hasn't been connected very long -- eg perhaps your tip is stale, but the peer you're choosing to evict is one you just connected to
< sipa>
sdaftuar: years ago i experimented with a outbound connection rotation system, which kicked just based on chances that depend on connection time
< sdaftuar>
or, perhaps your network was down for a few hours (say, you're running on a laptop) and then when you start up, you detect a stale tip -- so you immediately flag one of your initial 8 peers for eviction, for basically no good reason
< sipa>
i believe a good approach was (by simulations, but i've lost all the code so it's hard to reproduce) to make the chance to disconnect proportional to time_connected^-0.8
< sipa>
sdaftuar: well if we're considering something like peer rotation anyway (at least, i am), then occasionally randomly disconnecting someone for no reason isn't too bad
< sdaftuar>
sipa: i haven't given a ton of thought to how to measure success/failure of general-purpose algorithms like that...
< sipa>
at least if it only happens occasionally
< sdaftuar>
ah, ok
< sdaftuar>
well, i think my PR won't behave *terribly*, under the assumption that it's okay for an occasional spurious disconnect
< sdaftuar>
though it could certainly use testing
< sipa>
and in general, your most recent connection is the least likely peer to be your best
< sdaftuar>
right, agreed. i think my PR does have an edge-case bug, where eg your first peer could be the one disconnected (in the situation where your network is down, then comes back up, and you briefly have just one peer that is fully connected--
< sipa>
so if we're aiming for simplicity, i don't think it'll be terrible under any circumstances
< sdaftuar>
it could be flagged for eviction)
< sdaftuar>
i was considering another approach to the same problem, where i just connect outbound to a 9th peer when the tip looks like it might be stale, and then at some point after that disconnect one
< sdaftuar>
with the idea that if no new info was learned from the 9th, then it should be the one disconnected
< sdaftuar>
i think that approach has the advantage of (a) being able to trigger more often, because it seems safer to connect to an extra peer than disconnect an existing one, so you can test for stale-ness with less conservatism
< sipa>
right
< sdaftuar>
and (b) it seems like the outbound peer selection logic will perform better if you connect to a new peer before disconnecting an old peer (due to the netgroup diversity requirement? if i understand it right)
< sdaftuar>
but i haven't quite worked out the implementation details yet
< aj>
jtimon: hmm, googling old issues doesn't seem like there's much consensus on changing RPC from decimal BTC values? #9855 was the most recent i saw, i think
< jtimon>
perhaps that's a good topic for the next meeting
< jtimon>
it was my understanding that everybody considered using BTC instead of satoshis a mistake, but perhaps it's just me and I'm just assuming the opinions of others
< aj>
jtimon: if i'm understanding what they're saying, sipa and wumpus seem to think string vs number is more important than btc vs satoshi
< jtimon>
also I think string vs numbers is more important when you have decimals. for integer satoshis I don't think anyone has a problem with them being numbers
< jtimon>
perhaps I'm wrong there too, not sure
< jtimon>
but for the getblockstats pr, I really think using integer satoshis is the right thing to do
< jtimon>
or the best option
< bitcoin-git>
[bitcoin] Varunram opened pull request #11553: [P2P] Throw Warning if -peerbloomfilters is enabled (master...peerfilters) https://github.com/bitcoin/bitcoin/pull/11553
< Varunram>
Hey guys! This is my first PR, please pardon me if I've done something wrong or haven't abided by any rules. Looking forward to your comments :)
< bitcoin-git>
[bitcoin] TheBlueMatt opened pull request #11554: Sanity-check script sizes in bitcoin-tx (master...2017-10-bitcoin-tx-script-sizes) https://github.com/bitcoin/bitcoin/pull/11554
< BlueMatt>
Varunram: heh, thanks for the pr, though sadly I dont think we can do that yet :(
< BlueMatt>
Varunram: note that the -peerbloomfilters option is mostly to prevent dos issues for nodes which cannot take the potential latency increases, and not to encourage people to stop providing bloom filters until we have a viable alternative :/
< Varunram>
BlueMatt: haha, no worries :) I was going through the issues with "good first issue" labels on them and this seemed to be a good place to start. And yeah, we certainly don't want people to stop providing bloom filters, that would take a hit on SPV clients as a whole
< BlueMatt>
heh, yea, maybe we should be better about tags...
< BlueMatt>
someone should probably remove the "good first issue" tag there....
< Varunram>
I'll rummage through the issues repo and contribute something more useful, meanwhile do I close the PR or?
< BlueMatt>
I mean you dont have to, but it'll sit for 6/12 months if you dont "waiting on someone to add a viable alternative"....
< Varunram>
I had a slight notion that it might be controversial, but gave it a shot (yolo lol)
< BlueMatt>
so probably easier to...
< Varunram>
got it, thanks!
< bitcoin-git>
[bitcoin] Varunram closed pull request #11553: [P2P] Throw Warning if -peerbloomfilters is enabled (master...peerfilters) https://github.com/bitcoin/bitcoin/pull/11553
< jb55>
seconded on adding more good first issue tags, I'm also interested in getting my feet wet..
< cfields>
sdaftuar: sorry for the late review
< * luke-jr>
wonders why dns seeder added a second copy of the service bits
< luke-jr>
dnsseed.bitcoin.dashjr.org is now servicebit-filter-enabled
< sdaftuar>
cfields: thanks for taking a look!
< cfields>
sdaftuar: don't kill me for that last comment... I know 11534 was my suggestion :\
< sdaftuar>
no problem, thanks for thinking about it. not sure if you saw the scrollback, but i'm actually working on an alternate implementation where we'd add a new peer first, and then figure out who to disconnect
< sdaftuar>
i *think* that will turn out to be simpler... but the code isn't done yet, so too early for me to be sure
< sdaftuar>
testing this stuff is all awful of course :(
< sdaftuar>
anyway i think #11490 is closer to done, so i'll address your comments there and hopefully we can get some final acks soon
< gmaxwell>
sdaftuar: I like adding a new peer first; then we won't potentially spend a long time with one fewer peer.
< cfields>
I see. The idea being that you may disconnect a node just to connect to his neighbor. But if you have a new connection first, you can make a better decision
< sdaftuar>
cfields: yeah. i don't know how compelling that reasoning is, but that plus not being able to be more aggressive about checking whether your tip is stale both seem like good reasons
< sdaftuar>
and what gmaxwell said ^
< gmaxwell>
this was, fwiw, how I always envisioned outbound rotation working, temporarily allowing one extra peer, then deciding which one to axe after you've talked to the new one.
< cfields>
sdaftuar: so should i let #11534 rest until you've tried out your new approach?
< sdaftuar>
cfields: yeah maybe give me another day or so to try to get a new approach working, and then we can compare and see which looks more viable?
< cfields>
ok, np
< sdaftuar>
so you don't waste your time on an approach we abandon
< cfields>
also, if our p2p was more spec'd out, I'd grumpily ask for a bip for #11490. But I guess it's not worth it since we already make a bunch of assumptions.
< sdaftuar>
i'm not averse to documentation, but do you think it needs a bip? i feel like peer selection is something that doesn't really require coordination across implementations
< cfields>
nah, i don't think it's really needed
< cfields>
not the peer selection though, the "you must send MAX_HEADERS_RESULTS or be disconnected" rule.
< sdaftuar>
oh, i forgot about that
< gmaxwell>
We alrady have a requirement for the max results, since our headers syncinc chaining depends on it, no?
< sdaftuar>
gmaxwell: yeah, that's basically how i looked at it
< sdaftuar>
i think matt observed that if we added a one-off getheaders message (eg with a hashstop) before we'd completed chain sync from a peer for some reason, then that would interact badly with the new logic
< gmaxwell>
since it's automatic outbound only it wouldn't harm monitoring tools or such that don't really implement the protocol correctly.
< sdaftuar>
but we don't do that... and i don't know why we would
< cfields>
ah, so it does
< gmaxwell>
sdaftuar: another point why its important that we don't evict half our outbound peers in 11490. Consider, say there is a new softfork we don't know about. At the moment, a chain violating it is a block ahead. We don't want the non-upgraded network to completely partition the upgraded network, because eventually the upgraded network will get ahead and we will switch to it.
< sdaftuar>
gmaxwell: great point. i should add a comment
< sdaftuar>
hmm, do you think protecting the first 4 who give us headers is reasonable then?
< sdaftuar>
(afk, back later)
< BlueMatt>
google translate appears to think #11555 is the guy asking how to create his own altcoin....someone wanna close it?
< gmaxwell>
sdaftuar: to be fair on my point, inbound connections would heal the partition, but I feel better if we can justify the behavior even in the impossible network of only hosts with outbound connections.
< meshcollider>
BlueMatt: heh I like his/Google's phrasing, "altcoin come alive and go free swimming"
< esotericnonsense>
does anyone have a good way to reproduce slow RPC behaviour? all I can think of at the moment is continually resyncing which doesn't seem like a good idea for my poor SSD. perhaps running a seperate thread that hits RPC with heavy queries over and over?
< aj>
jtimon: i think the drawback for numbers vs strings is that even without a decimal point, "integers" are treated as doubles in javascript which makes it ambiguous at best in json generally
< gmaxwell>
some json implementations manage to use floats for numbers too! :(
< jtimon>
aj: yeah, perhaps all data in getblocksstats should be returned as strings, I still prefer satoshis over in BTC in that case
< jb55>
I find myself using sed to wrap numbers in strings before I process them with jq...
< jtimon>
with that PR, I'm also kind of stuck with testing...I need to add at least one segwit tx to test some of the functionality, but some size and feerate functionality is currently not being tested due to sigs not being deterministic in size,I thought about putting hardcoded private keys in the test, but nobody confirmed that it makes any sense
< jb55>
jtimon: wouldn't it make more sense to just to dump N verbose blocks and build analysis tools externally? I guess that would be a bit slower...
< jtimon>
jb55: mhmm, I'm talking about the tests, but yeah, didn't thought about that...hardcoded blocks should do the trick and additionally also solve any potential concern with coin selection being non deterministic too, in case more complex tests are needed in the future. Great suggestion, now I feel bad about not thinking about it myself, but good about you not waiting for me to discover that possibility on my own
< jb55>
just came to mind because I was trying to write a script that gave me the average block times the other day...
< jtimon>
are there any other tests that use hardcoded blocks that I can copy from ? I guess the answer is no since https://github.com/bitcoin/bitcoin/pull/8994 which changes the genesis block for all functional tests is passing, but it was recently brought to my attention that hardcoding txs (including coinbases) should be enough even to construct a compatible even with a hf that completely changes the proof of work rules from genesis
< jtimon>
block (such as regtest/custom) [isn't rusty around? it was him who reminded me in a completely unrelated conversation, I vaguely remind having known this before, or perhaps was just a deja vu]
< jtimon>
or perhaps it's been too long since last time I tried the extended tests (including pruning)...
< jtimon>
(for that PR)
< jtimon>
I'll stop overtly and subconsciously review begging now...