< bitcoin-git>
[bitcoin] tecnovert opened pull request #16412: net: Make poll in InterruptibleRecv only filter for POLLIN events. (master...bitcoin-poll) https://github.com/bitcoin/bitcoin/pull/16412
< promag>
we already have "experimental" features and IMHO the lack of this feature invalidates some multi wallet usages
< sdaftuar>
#proposedmeetingtopic -blocksonly not being a hidden option in 0.18.1
< wumpus>
please, let's try to cut 0.18.1 at some point
< wumpus>
and not keep adding things t oit
< promag>
just asking, again :P
< promag>
what about #16322
< gribble>
https://github.com/bitcoin/bitcoin/issues/16322 | wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction by promag · Pull Request #16322 · bitcoin/bitcoin · GitHub
< promag>
is has "Needs backport"
< promag>
it's not a clean backport
< promag>
(by far)
< wumpus>
ok, maybe it can wait until 0.18.2 then
< promag>
+1
< MarcoFalke>
Wat?
< promag>
I mean I agree
< MarcoFalke>
I don't
< promag>
ok then :D
< MarcoFalke>
Users are throwing away coins due to a bug in our software
< wumpus>
ok, 0.18.1 plans for today are off...
< wumpus>
*resets tree*
< MarcoFalke>
sorry :(
< promag>
backporting that requires some refactors
< MarcoFalke>
promag: Would the backport be clean if you also cherry picked the change where the txerros are moved to the new file?
< promag>
or rewrite the fix in 0.17 without those refactors
< wumpus>
it's a compromise with how bad the current bugs are that require the 0.18.1 release in the first place, if you think this bug is *much worse* than what the fixed already in it fix, well then it makes sense to wait
< promag>
i mean 0.18
< wumpus>
if not, it just exposes users to other bugs for longer
< promag>
MarcoFalke: I'll have to check that
< MarcoFalke>
The maxtxfee is not a regression. I has existed for all time, I think. I am glad that promag finally fixed it
< wumpus>
I'm also glad that he fixed it, don't get me wrong
< wumpus>
the tests don't have the sameconcerns, backport as much refactors as you need there
< wumpus>
looks like #16412 needs to hold up 0.18.1 too
< gribble>
https://github.com/bitcoin/bitcoin/issues/16412 | net: Make poll in InterruptibleRecv only filter for POLLIN events. by tecnovert · Pull Request #16412 · bitcoin/bitcoin · GitHub
< wumpus>
sdaftuar: ok, I see #15990 has been backported to 0.18.1, the intent of your meeting topic is whether it should be hidden again?
< bitcoin-git>
[bitcoin] promag opened pull request #16414: 0.18: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (0.18...2019-07-backport-16322) https://github.com/bitcoin/bitcoin/pull/16414
< promag>
MarcoFalke: wumpus: wip backport ^
< promag>
let me know if I should squash
< kallewoof>
I would like to request that #16411 is added to Chasing Concept ACK (will probably crash before meeting starts).
< sdaftuar>
wumpus: my topic suggestion is more to discuss the implications of there being more -blocksonly listening nodes out there
< wumpus>
okay,makes sense
< warren>
sdaftuar: is their motivation bandwidth reduction?
< dongcarl>
wumpus: reading the addrv2 BIP now... why VARINT for time and service bits, why not just u64s?
< sdaftuar>
warren: presumably?
< wumpus>
dongcarl: to save space in the common case
< gleb>
There are also attack vectors implied by transaction relay topology leaks, but I don't know for now how practical those are.
< wumpus>
(bandwidth)
< sipa>
who is they?
< gleb>
-blocksonly listening nodes I think?
< sipa>
i mean in warren's question, "they" seems to refer to persons
< wumpus>
dongcarl: these are structures which are potentially repeated many times in a packet, so every byte saved is good
< wumpus>
dongcarl: why the preference for u64s?
< dongcarl>
Eh, just thought it's simpler
< wumpus>
one thing that addrv2 does, too, is represent ipv4 addresses as four bytes
< dongcarl>
wumpus: Oh, that's good
< sipa>
a varint for a 33-bit value is also 8 bytes
< sipa>
*9 bytes
< wumpus>
so if you, at the same time, blow up time to 8 bytes...
< sipa>
assuming it is refering to the usual protocol compactsize encoding, not our internal varint encoding
< wumpus>
sipa: yes, one suggestion was to quantize time, this would avoid some topology leak attacks as well as save space
< sipa>
i don't see the topology argument; it's just the data inside the addresses, not their encoding that matters?
< wumpus>
precise last-seen times allowed for that
< wumpus>
AFAIK there was already a mitigation against that, though
< dongcarl>
Right, there seems to be quite a few possibilities for the "time" field discussed in the BIP
< wumpus>
"(gmaxwell) If you care about space time field could be reduced to 16 bits easily. Turn it into a "time ago seen" quantized to 1 hour precision. (IIRC we quantize times to 2hrs regardless)."
< dongcarl>
1. UNIX epoch 2. last seen 3. whether to quantize
< wumpus>
oh, right
< wumpus>
I kind of forgot that he made the "time ago" suggestion, it seems weird for gossiping
< dongcarl>
I feel like epoch quantized to 1 hr precision sounds good...
< wumpus>
dongcarl: in the BIP itself it simply encodes absolute time, those considerations are separate
< dongcarl>
not sure what is meant by (we quantize time to 2hr regardless)
< wumpus>
apparently we already do that in the net code
< wumpus>
to mitigate the topology leak attack
< wumpus>
but I don't know where tbh
< meshcollider>
meeting?
< dongcarl>
sounds like I need to do more documentin'
< dongcarl>
hi
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jul 18 19:00:28 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< wumpus>
luke-jr: not sure separating them out is a key to get more interest imo
< luke-jr>
expiry helps remove unconfirming txs from RAM etc
< MarcoFalke>
luke-jr: Yeah, if removing expiry is so controversial, then yes
< wumpus>
anything that touches policy is probably somewhat controversial
< sipa>
another approach is to make the marginal-cost-for-replacement go to 0 after some time
< sdaftuar>
I think expiry performs an important job, albeit not perfectly, which is handling diverse node policies in a semi-reasonable way
< luke-jr>
but what if the sender doesn't replace?
< MarcoFalke>
luke-jr: YOu can not expect to use that ram anyway, since you set the max usage to well the max usage
< sipa>
yeah, i'd need to think more about the impact there
< luke-jr>
MarcoFalke: it might not always be maxxed out
< jnewbery>
sipa: that doesn't solve the case of txs being pinned
< sipa>
jnewbery: ah yes
< MarcoFalke>
sipa: I don't see this as an issue in the network as of today (it is mostly theoretical)
< luke-jr>
MarcoFalke: it's an issue because people are designing around it
< sdaftuar>
the issue of diverse node policies is an issue
< midnightmagic>
I was going to say, expiry is super useful..
< sdaftuar>
if we deploy this now, then in the future when we make a policy change, we'll have a problem to think about
< sdaftuar>
which we won't have a good way to solve
< MarcoFalke>
sdaftuar: Agree that it might be premature to remove expiry right now, but in the long term it could at least be limited to high-fee txs
< wumpus>
what is the primary motivation to remove expiry?
< sipa>
sdaftuar: when a policy change happens, there is always a shutdown+restart involved, no?
< sipa>
sdaftuar: so there is a load from mempool.dat in between?
< sdaftuar>
the issue is around what happens to old nodes
< MarcoFalke>
If miners are running the default settings, they might throw away income. If they are not, then nodes have a false sense of what can be replaced
< sdaftuar>
which will not have the new rule, but will also not exprie things that are not getting mined
< luke-jr>
not just old nodes. even updated nodes don't have a common policy
< sdaftuar>
luke-jr: i agree with that too
< MarcoFalke>
So the rbf pinning issue jnewbery mentions is not solved if miners are keeping the tx and you can replace it in your own mempool
< wumpus>
so dropping expiry would mean that some transactions could get stuck in the mempool literally forever
< sdaftuar>
wumpus: exactly
< sipa>
i feel 2 weeks is sufficiently long that it isn't actually affecting anything that will likely confirm
< sipa>
do we have numbers otherwise?
< wumpus>
even an expiry of, say, 2 month would be better than none at all in that regard
< sipa>
like at what rate do transactions get expired from common mempools?
< sipa>
and do any of them still confirm after?
< sipa>
or maybe even better, how often do transaction expire from the mempool, and then re-enter it?
< wumpus>
would be useful to have that information
< kallewoof>
sipa: I can get those numbers
< sipa>
that'd be great to have in this discussion
< luke-jr>
What problem is this trying to solve?
< kallewoof>
sipa: They're not very large, from what I have seen
< MarcoFalke>
kallewoof: The number of expiries or the number that are mined after expiry?
< sdaftuar>
i think this needs to be thought about from the perspective of older software as well
< sdaftuar>
if you only analyze this from the perspective of the latest version of Bitcoin Core, for instance, you only get a one-sided view
< kallewoof>
MarcoFalke: I can fine tune it with some tweakery. Will not be done today, but can do tomorrow
< sipa>
but it doesn't have to be large; say if 5 transactions expire per day, but 4.5 of those then re-enter the mempool again, that's evidence that due to rebroadcasting the expiration is effectively ineffective
< * sipa>
likes "effectively ineffective"
< wumpus>
hehe
< kallewoof>
MarcoFalke: To clarify, I can fine tune it to count txs that do get mined after being purged from mempool at least once.
< sdaftuar>
can you also do that from the perspective of an 0.15 node?
< kallewoof>
sdaftuar: no
< Raystonn>
It ineffectively affects the mempool, which results in not much net effect.
< kallewoof>
sdaftuar: well, actually yes, but not without your help.
< sdaftuar>
kallewoof: my data is probably insufficient too, unfortunately
< sdaftuar>
i am not really sure i guess
< kallewoof>
sdaftuar: i thought it recorded everything
< kallewoof>
sdaftuar: besides, having the simulation mode alone means all you have to do is fake the time and throw the txs at the right moment and the node should do the expiration on its own
< sdaftuar>
i don't know offhand how representative its outbound peers are
< sdaftuar>
as an example, i was surprised today to discover that my 0.12 node accepted and then expired some transactions on a particular day in May
< sdaftuar>
it looks like on May 5, it received some very low-fee transactions, which were accepted to the mempool as "priority" transactions
< sdaftuar>
i was somewhat shocked that its peers (it only has the 8 outbound) would relay such things to it
< sdaftuar>
but i guess it has some peer diversity
< MarcoFalke>
sdaftuar: Those are probably valid txs, so I wouldn't call that "shocking"
< sdaftuar>
MarcoFalke: i was shocked that the policy diversity on the network was so strong that 8 random peers would include some that aren't enforcing the minrelayfee we have had in place since 0.15
< luke-jr>
mempool still had a priority exception in 0.15? O.o
< MarcoFalke>
Ok, fine.
< sdaftuar>
i think 0.15 was when we first got rid of it
< kallewoof>
I looked at our email convo and it looks like I am waiting for you to give me a snapshot. :)
< sdaftuar>
kallewoof: oops, thanks for the reminder :)
< MarcoFalke>
I think this is too controversial right now, so I will let it sit for a while
< MarcoFalke>
and revisit later
< MarcoFalke>
Can we chat about 0.18.1?
< jnewbery>
MarcoFalke: after some digging, it turns out those transactions were actually from 2017. Someone had dug them up and rebroadcast them
< MarcoFalke>
sdaftuar and me had a topic on 0.18.1
< wumpus>
#topic 0.18.1
< MarcoFalke>
jnewbery: Which reads to me like a reason to remove expiry, but anyway
< Raystonn>
A hodler found and opened his wallet again.
< sdaftuar>
i wanted to discuss the impact of the -blocksonly change
< MarcoFalke>
(Think how shocking it would be if you see a tx confirm 2 years after you "cancelled" it)
< wumpus>
apparently, the maxtxfee fix is harder to backport than expected
< MarcoFalke>
wumpus: I will take a look as well, but if it is really too hard I am fine with 0.18.2
< Raystonn>
The only way to guarantee a transaction will not confirm once it has broadcast is to spend the relevant UTXOs in another.
< wumpus>
oh, looks like promag did already open a backport PR: #16414
< gribble>
https://github.com/bitcoin/bitcoin/issues/16414 | 0.18: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction by promag · Pull Request #16414 · bitcoin/bitcoin · GitHub
< wumpus>
in any case as this is a non-clean backport it does need review and testing
< wumpus>
more than had it been a clean backport, at least…
< MarcoFalke>
Three people wrote tests for it
< MarcoFalke>
So #16412 will get in as well?
< gribble>
https://github.com/bitcoin/bitcoin/issues/16412 | net: Make poll in InterruptibleRecv only filter for POLLIN events. by tecnovert · Pull Request #16412 · bitcoin/bitcoin · GitHub
< wumpus>
MarcoFalke: it should, it's a small and obvious change, that fixes a real problem, and is easy to backport
< MarcoFalke>
#15911 doesn't make progess, so it'll have to wait
< wumpus>
it's been waiting for author for a while
< MarcoFalke>
agree
< wumpus>
even though my comment is trivial
< fanquake>
*can
< wumpus>
ok, moving
< wumpus>
ok let's go to sdaftuar's topic
< promag>
hi
< wumpus>
-blocksonly is now a non-hidden option in 0.18.1
< sdaftuar>
so historically, i believe we have not made the -blocksonly option more widely known because we don't have good protections in place for a network where many listening nodes are not relaying transactions, i think
< sdaftuar>
now that we're doing it, i think we should make sure there are not unintended side effects
< MarcoFalke>
sdaftuar: It is mentioned on bitcoin.org
< MarcoFalke>
And has been for years
< sdaftuar>
for instance, if we see a rise in -blocksonly listening nodes, then right now we have no protections in place for ensuring connectivity to transaction-relaying peers
< jonasschnelli>
fee-estimations are not possible with -blocksonly? right?
< sdaftuar>
MarcoFalke: i was not aware! anyway, perhaps that doesn't matter much, i think we should still be concerned about making our software more robust
< MarcoFalke>
agree
< jnewbery>
jonasschnelli: correct, but I don't see how that's relevant
< kallewoof>
jonasschnelli: fee estimations use blocks only, so it should be possible
< wumpus>
I don't personally think unhiding the option in help is going to make that much of a difference, it's been well-known
< warren>
maybe an informational tx relaying service bit?
< jnewbery>
kallewoof: fee estimation requires a mempool
< sdaftuar>
wumpus: fair point, perhaps our choice here is not relevant
< luke-jr>
do they dset NODE_NETWORK?
< warren>
or informational not-tx-relaying service bit?
< sdaftuar>
luke-jr: yes
< kallewoof>
jnewbery: weird. i thought it only used blocks. maybe it changed since last i looked at it.
< sdaftuar>
i think we could make our p2p code smarter to ensure we have "enough" tx-relaying outbounds, but this work needs to be done, and i think we should prioritize it
< luke-jr>
warren: I don't think we gain anything by making it a negative bit
< MarcoFalke>
sdaftuar: Could you create a brainstorming issue for that?
< sdaftuar>
also, old software has no protections, i don't know what to do about that. presumably we're still a while away from there being a large fraction of listening nodes being blocksonly, so we have some time
< sdaftuar>
MarcoFalke: sure
< wumpus>
is blocksonly really that popular?
< jonasschnelli>
jnewbery: I think its relevant because it reduces the usability of blocksonly&wallet significant. I's already an expert features and thouse who are know the blocksonly feature.
< sdaftuar>
wumpus: i forgot to mention why i brought this up
< gleb>
I'll try to simulate how "half of nodes turning blocksonly" or something like that might affect tx relay latency or compact blocks relay latency, let me know if you have other metrics in mind
< sdaftuar>
i encountered a random blocksonly listening node when testing something recently, and i was surprised
< wumpus>
sdaftuar: that's very anocdotal but yeah :)
< MarcoFalke>
Could a short-term fix be to disconnect a random outbound if all outbounds are blocksonly?
< sdaftuar>
wumpus: so i don't know how common it is now, but when i realized we were unhiding the option and recommending it in our docs, i figured its popularity could rise
< warren>
I had been running a blocksonly node for years
< wumpus>
if you find one by accident then it's probably more common
< jonasschnelli>
MarcoFalke: wouldn't that require a new service flag (or a split of NODE_NETWORK / RELAY)?
< jonasschnelli>
(to work reliable)
< warren>
not "reliable" but at least you could be told all your peers aren't relaying
< sdaftuar>
jonasschnelli: right now, nodes running with -blocksonly achieve that by setting the fRelayTxes=false in the VERSION message
< sdaftuar>
so we can detect it on connection
< wumpus>
right, you can detect it for connected nodes through the version message
< jonasschnelli>
I see. So we could just disconnect fRelayTxes=false up to a certain threshold
< wumpus>
for outbound only I hope
< sdaftuar>
i wonder if the dnsseeds people run are tuned to look for this?
< * sdaftuar>
stares at all of you
< * jonasschnelli>
checking....
< sipa>
i doubt it
< jonasschnelli>
it's not checking the fRelayTxes
< sdaftuar>
matt seems skeptical this is a good idea, anyway i just wanted bring up the topic so others can think about it
< wumpus>
I'm not sure it should filter out all nodes that are blocksonly
< cfields>
sdaftuar: why would it? Seeds are primarily hit during IBD.
< sdaftuar>
cfields: i think in practice we hit the seeds a lot now, actually, but i think that's also a bug
< wumpus>
the main task of the P2P network is relaying blocks, after all, sure there need to be nodes that relay transactions, but I think it's a bad idea to just ignore nodes that don't
< wumpus>
cfields: right
< sdaftuar>
wumpus: yeah i think we can tolerate some level of blocksonly peers, but not all of our outbounds
< wumpus>
for IBD it definitely doesn't matter
< sdaftuar>
i'm not sure where to draw the line
< wumpus>
sdaftuar: agreed
< wumpus>
(it ignores transactions during IBD, after all)
< cfields>
sdaftuar: agree that it's something that should be handled, though.
< MarcoFalke>
Heh, so it would be helpful to connect to blocksonly in IBD
< cfields>
s/handled/tolerated/
< kallewoof>
50/50 for tx relaying nodes, anything-goes for blocksonly nodes?
< luke-jr>
MarcoFalke: well, except some people use blocksonly as a kind of "super low bandwidth mode" :P
< wumpus>
well if you enable pruning too...
< warren>
perhaps many using blocksonly would be happy with erlay
< MarcoFalke>
I hope they don't accept incoming then
< sdaftuar>
btw if anyone is interested, i think i've made a lot of progress on refactoring our code to support package relay. Review welcome! #16400 / #16401
< bitcoin-git>
[bitcoin] TheBlueMatt closed pull request #16324: Get cs_main out of the critical path in ProcessMessages (master...2019-07-peerstate-initial-moves) https://github.com/bitcoin/bitcoin/pull/16324