< sdaftuar>
if we're making a regular outbound connection and our peer has fRelayTxes=false when we connect (ie because that node is running in -blocksonly mode, i'm guessing?) should we disconnect?
< sdaftuar>
now that -blocksonly is not a hidden option, i think we should probably do something to ensure we have sufficient tx-relay peers
< sdaftuar>
opinions?
< jamesob>
sounds reasonable to me. I'd say this would be especially important if we're more lenient with fRelayTxes=false peers re: eviction based on some kind of inactivity ("I haven't received an INV from this peer in n minutes") but I don't think that's the case at the moment
< jamesob>
the rationale being that it'd take fewer resources to sybil broadly with a bunch of -blocksonly nodes, but again I don't think that's the case atm
< sdaftuar>
i don't think there's a partition risk introduced by this, i'm more thinking that we have no protections in our code against our 8 regular outbounds all being run in -blocksonly mode and so there's an edge case where we are surprised to not receive transactions at all
< sdaftuar>
i'm not sure how much transaction-relaying peers is sufficient, but it'd be easy to detect a blocksonly peer when we're looking for a regular peer and just disconnect, i think. might be too heavy-handed though?
< jamesob>
is ensuring some min ratio of regular peers too special casey?
< sdaftuar>
well, there are only 9 values to choose from when it comes to the number of blocksonly peers you'd tolerate as a "regular" outbound, so i think we can just pick where to set that threshold :)
< sdaftuar>
(i don't think we should include inbounds in that calculation)
< gleb>
hi
< gleb>
sdaftuar: Why there is no explicit p2p message when "fRelayTxes=false"?
< sdaftuar>
it's a hack -- we use a bit added to version messages when bip37 was rolled out to communicate that we don't want to enable transaction relay
< gleb>
So why we don't want to keep using this bit when identifying a fRelayTxes=false on other side?
< sdaftuar>
because bip37 allows for transaction relay to be re-enabled later in the connection's lifetime
< sdaftuar>
i'm not sure whether bip37 support is currently default on or off right now in our software, but we should do something more explicit for blocksonly connections in the future, i think.
< gleb>
Oh, I See. So something like an explicit message? :)
< sdaftuar>
yes
< lightlike>
sdaftuar: looks like bip37 support is on per default, but will possibly be disabled if #16152 is merged
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #16380: Remove unused bits from the service flags enum (master...1907-netRemoveUnusedServiceBits) https://github.com/bitcoin/bitcoin/pull/16380
< MarcoFalke>
Run a getblockbyheight locally and you get the error
< wumpus>
yea, 'waiting for author' just means 'author needs to reply', it doesn't mean that the author needs to do everything (including cleaning toilets) that the reviewers propose :p
< wumpus>
it's fiine to say 'this is out of scope of this PR'
< MarcoFalke>
A p2p network change is not a command line change
< wumpus>
I'd even encourage that to make sure that progress is made and things don't get held up in a back-and-forth forever
< wumpus>
MarcoFalke: definitely
< MarcoFalke>
If there is feedback and multiple reviewers agree on it, it shouldn't be ignored
< MarcoFalke>
If it was a whitespace nit or something, yeah maybe ignore it
< wumpus>
it should never be *ignored*
< wumpus>
saying 'no' is ok, ignorning not
< MarcoFalke>
Agree
< MarcoFalke>
Knowing the reason that some feedback has not been addressed gives helpful insights
< MarcoFalke>
s/that/why/
< wumpus>
I mean reviewers spend time in writing review comments so the least you can do is respond to it, with reasoning