< fanquake>
jonatack: are you using the (I assume self-built) Clang master branch for development/review?
< jonatack>
fanquake: i changed from using debian stable to debian testing and added "deb http://apt.llvm.org/unstable/ llvm-toolchain main" to etc/apt/sources.list
< fanquake>
Right. Why not use Clang 9/10 though?
< jonatack>
No strong opinion yet, updated only last weekend. Which do you think is best for development/review?
< jonatack>
My motivation was for testing the cfields' span PR with -Wdangling and have clang-format work again (it's currently broken for clang < 9)
< fanquake>
Preferably a compiler that in not unreleased/WIP. 9 or 10 shouldn't make much difference. I was just curious after seeing your review comments.
< jonatack>
Ok. FWIW #19454 fixes .clang-format for clang < 9, e.g. Debian stable
< sipa>
if a client miabehaves they should be disconnected and/or banned immediately
< sipa>
if they don't, we ahouldn't confuse things by maybe later doing that when some threshold is reached
< sipa>
it just obscores things for no benefit
< someone235>
Yeah, if we're talking about not talking with broken nodes I tend to agree
< sipa>
yeah see jonatack's link
< someone235>
But how will you deal with attackers? Like people who sends you full invalid blocks?
< someone235>
*send
< sipa>
ban them
< sipa>
immediately
< someone235>
oh right
< sipa>
but also
< someone235>
and bans are kept between reconnects
< someone235>
and restarts
< sipa>
there are plenty of ways to trivially DoS attack nodes by doing completely legitimate things
< sipa>
that we can't ban for
< sipa>
the eventual way to deal with that imho is tracking how muxh resources we spend on behalf of each peer
< sipa>
and if we run out, deprioritize or disconnect the worst offenders (without banning)
< bitcoin-git>
[bitcoin] kiminuo closed pull request #19466: Use operator/ in fs::absolute to prepare for C++17 (master...feature/2020-07-08-fs-paths) https://github.com/bitcoin/bitcoin/pull/19466
< willcl_ark>
MarcoFalke: Right. I'm looking at #19388 a bit more, and it seems to me undesirable that we'd now require e.g. afl-gcc for all the gcc builds if fuzzing harnesses were built by default.
< MarcoFalke>
Oh, if you don't need instrumentation, you should be able to use any cpp compiler
< MarcoFalke>
The fuzz engine only provides the main function, but the individual fuzz tests don't know that. They can be compiled with any cpp compiler
< bitcoin-git>
[bitcoin] fanquake closed pull request #18604: Display command line usage to console without requiring X Windows (master...UsageToConsoleIfNotWin32) https://github.com/bitcoin/bitcoin/pull/18604
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19474: doc: Use precise permission flags where possible (master...2007-docPerm) https://github.com/bitcoin/bitcoin/pull/19474
< bitcoin-git>
[bitcoin] Mohamed-94 opened pull request #19475: script: Speed up default time-out and Container CPU usage (master...patch-1) https://github.com/bitcoin/bitcoin/pull/19475
< instagibbs>
it's hard to get your hands on large amounts unless you "know people"
< phantomcircuit>
instagibbs, it's extremely easy to mine a huge amount of tnbtc if you modify a stratum proxy very slightly
< instagibbs>
someone is warp mining this morning AFAICT
< cfields>
jonatack: that warning has me completely stumped, and now I'm curious. Building clang from source now to play along.
< jonatack>
cfields: idk if it's worth it. i didn't install from source. added "deb http://apt.llvm.org/unstable/ llvm-toolchain main" to my etc/apt/sources.list
< cfields>
ack
< jonatack>
fanquake suggested this am that i move back to clang stable 9 or 10 for reviewing purposes
< cfields>
jonatack: do you get a ton of warnings? Or just that one?
< jonatack>
just the one. i agree with you that it doesn't make sense.
< cfields>
you actually checked out the commit, right? Didn't c/p from github?
< jonatack>
yes, i always do. build several times with make distclean to be sure it was really doing that, and also compared with a clean gcc 9.3 build
< jonatack>
built*
< cfields>
So weird!
< cfields>
We'll see if I can reproduce from a git build.
< sipa>
jonatack: and you only get the warning on the new code introducsd to attrihutes.h?
< sipa>
not anywhere else?
< jonatack>
I'm afraid I've led you into a goosechase cfields. sipa: correct, but i've only been reviewing with this version of clang since sunday.
< cfields>
jonatack: no worries, I needed to build llvm to test recent lld anyway. That's the only reason I jumped to compile so quickly.
< jonatack>
i was on debian clang 6 which does not have -Wdangling and wanted to test cory's PR
< jonatack>
cfields: yes. LMK
< jonatack>
rebuilt just now again, saw it again. but also apt-update has new clang updates. installing and rebuilding.
< jonatack>
cfields: upgraded from 11.0.0-++20200708111116+1f84ace3c72-1~exp1~20200708091733.3345 to 11.0.0-++20200709111134+dc4a6f5db4f-1~exp1~20200709091753.3347 and the warning is gone.
< sipa>
chased goose is best goose
< jonatack>
:D
< cfields>
jonatack: heh, well that's rather unsatisfying
< cfields>
jonatack: igoring all that, thanks for jumping through hoops to test :)
< jeremyrubin>
#proposedmeetingtopic small clarification on the goals of the mempool project (cap at 5 mins meeting time)
< luke-jr>
would be nice to get the build tarballs fixed finally too <.<
< meshcollider>
hi
< pinheadmz1>
hi
< wumpus>
#topic clarification on the goals of the mempool project (jeremyrubin)
< jeremyrubin>
Yeah just quick;
< jeremyrubin>
So I think that there's some confusion or at least co-mingling of concerns based on how I've presented things
< jeremyrubin>
A lot of the motivation is to reduce the memory usage in the mempool
< jeremyrubin>
and to better bound the algorithmic complexity on functions
< jeremyrubin>
So that one day, maybe, we could lift some restrictions (e.g., descendents)
< jeremyrubin>
It's less so "make mempool faster now" performance motivation.
< wumpus>
ok good to know
< jeremyrubin>
In a lot of cases I think it would be fine to accept a *slower* score on some benchmark (or at least the same) when the goal of the PR is reducing memory
< jeremyrubin>
especially in cases where there may be a distant invariant which is currently protecting us from a "crash the network" style DoS
< luke-jr>
if those are the goals, I wonder if it might make sense to move complexity into the miner
< wumpus>
we could have benchmarks that measure memory usage I suppose
< luke-jr>
but that could interfere with compact blocks
< jeremyrubin>
luke-jr: this does make sense for certain things.
< luke-jr>
I wonder if it'd be possible to support a build without mining support that performs better
< jeremyrubin>
I think one of the things that is difficult in particular is that we don't have a great set of adversarial benches for the mempool
< jeremyrubin>
And you need both whole-system and unit tests for the functions
< wumpus>
I think there's something of a conflict there, miners want the block selection to be as fast as possible, while other node users would want to reduce memory usage for the mempool as much as possible
< jeremyrubin>
And both asymptotic and bounded size
< cfields>
wumpus: +1
< luke-jr>
wumpus: by making mining build-time-optional, we could possibly get both cheaply?
< ariard>
I'm not sure about reducing the memory usage, what the relation between size of your mempool and perf of fee estimation ?
< luke-jr>
I wonder if there's a way to change class sizes at runtime in C++
< jeremyrubin>
Reducing memory usage is related to DoS primarily
< wumpus>
that's kind of annoying for testing but yes
< jeremyrubin>
So the goal is to eliminate the class of vuln
< luke-jr>
jeremyrubin: eh? users can always adjust their mempool size
< jeremyrubin>
I don't care about performance here that much, even a 2x slower mempool with less DoS surface is probably fine
< wumpus>
it doesn't help if only the miners would have the vulnerability though
< sipa>
mempool size _predictability_ is a DoS concern
< jeremyrubin>
luke-jr: no for the algorithms themselves
< luke-jr>
reducing mem usage would just mean more txs per MB
< jeremyrubin>
not for the mempool size itself
< ariard>
I think there is a confusion there with memory usage of caching data structure for update and overall mempool size
< sipa>
e.g. improving average memory usage on average, but worsening it under a particular edge case might constitute a vulnerability
< sipa>
and was trying to make the accounting allocator not track copies of containers, which is a feature of the C++11 allocator infrastructure
< wumpus>
I *really* think we should wait with bumping gcc versions until c++17 requirement
< sipa>
but it seems gcc 4.8 ignores it or otherwise fails to implement it correctly
< wumpus>
which isn't too far away, just one major version
< sipa>
yeah
< luke-jr>
sipa: not the end of the world if it's wrong?
< sipa>
but i've just noticed at appveyor also fails at it :s
< sipa>
luke-jr: could cause UB
< luke-jr>
hmm
< sipa>
if used in totally reasonable ways
< sipa>
(but probably not in my actual PR)
< wumpus>
not that I'm opposed to bumping it sooner if it's eally required for something, but it seems a waste of time to discuss minor version bumps if a big one is around the cornr :)
< luke-jr>
when C++20? :p
< wumpus>
in 2025
< * luke-jr>
found C++20 to be required for totally obvious/expected features the other day :/
< sipa>
luke-jr: hard to talk about things that don't exist
< cfields>
luke-jr: get sipa to backport for you like Span :p
< wumpus>
MarcoFalke: okay maybe I'm misreading the issue then
< aj>
8.3 is in debian stable as the default gcc, only gcc 6 in oldstable
< luke-jr>
aj: RHEL tends to be the bottleneck
< wumpus>
in any case there is going to be a large bump
< MarcoFalke>
sipa: Maybe rebase to see if msvc can compile it with C++17. If not, there is something else to look into first anyway. Pretty sure the 3 months will pass quickly ;)
< aj>
luke-jr: it has software collections now so you get new gcc/clang on old rhel pretty easy
< sipa>
RHEL8 has gcc 8.2
< luke-jr>
aj: oh!
< sipa>
RHEL7 uses gcc 4.8 by default
< luke-jr>
do we care about old stables now?
< sipa>
(we've strayed a bit from the topic, but that's ok unless someone has something else)
< cfields>
sipa: I was under the impression you weren't supposed to inherit from std::allocator in c++11.
< cfields>
ok, will look more after the meeting.
< instagibbs>
Ok, so recently I wrote a one-off zmq notification for mempool evictions, which are currently not covered. Other people have more exntensive ideas: https://github.com/bitcoin/bitcoin/pull/19476
< instagibbs>
shuckc, can you speak to motivation/usage?
< wumpus>
luke-jr: maybe, it's not that much more difficult, especially nowadays with the extreme availability of VMs etc
< cfields>
sipa: no idea if that's useful, need to spend a whole lot more time understanding what you're doing :)
< shuckc>
We track a huge number of wallets, keeping mempool contents synchronised is tricky given incomplete notifications, and difficult to sychronise between api and zmq notifications
< shuckc>
seems like an opportunity to cover off a lot of the edge cases in one go
< instagibbs>
so ideally you could getrawmempool like once, then use zmq notifications to track delta, then maybe call getrawmempool when something drops for whatever reason, and be able to figure out "where" in that notification stream the snapshot is from
< luke-jr>
instagibbs: you linked the same PR twice
< instagibbs>
sure luke-jr so alternative would likely look like promag pr i linked
< instagibbs>
maybe long polling
< wumpus>
only in pretty extreme circumstances it sometimes drops a packet
< sipa>
ZMQ has no reliability _guarantees_
< wumpus>
and in that case there needs to be a way to resynchronize, anyway, as instagibbs says
< luke-jr>
wumpus: I used it for low traffic on a reliable network, and it still lost stuff regularly
< sipa>
but absent overflow conditiins, it is very reliable
< wumpus>
notifications have sequence numbers to be able to detect that
< sipa>
luke-jr: hmm, ok
< instagibbs>
and avoiding "lots" of getrawmempools I guess is hte biggest goal
< wumpus>
sipa: yes, in the general case it is very reliable
< promag>
the biggest problem if you have to take the client offline for a bit
< shuckc>
ZMQ generally work as well as any other pubsub system so long as you have the high watermark set sufficiently high, and you are not trying to consume slower than the publisher is producing. I don't see that as a particular obstancle
< wumpus>
unless your client is not consuming the notifications reliably: there can't be an infinite buffer
< wumpus>
(unless you'd spool to disk or something like that)
< promag>
shuckc: zmq pub doesn't hold msg if client is offline.
< wumpus>
but I don't think adding yet another notifications system with mail-like reliablity is really what we want
< shuckc>
if your client goes away, you are going to have to hit getrawmempools for sure. but would like to avoid those calls in the general case as big result set (even when brief)
< wumpus>
I mean there's mq systems like rabbitmq that guarantee 100% reliability
< promag>
the approach in #19476 avoids periodic getrawmempools
< jnewbery>
why not have ZMQ log every txid it sends a notification for along with seq number. If your client detects a drop it can consult the log and query the mempool for that txid?
< wumpus>
but I mean, how many of these things do you want to integrate with
< instagibbs>
promag, your rpc could be maybe generalized into block hash announcements too, connect/disconnect
< instagibbs>
jnewbery, it has as seq number
< instagibbs>
but right now it's a hodgepodge of endpoints
< wumpus>
yes, it has a seq number
< instagibbs>
and missing eviction entirely
< jnewbery>
right, we're talking about two things here. Let's assume that your eviction PR is merged
< wumpus>
all the zmq endpoints have seq numbers
< shuckc>
if eviction PR is merged, the remaining issues are:
< wumpus>
I'mnot sure these are actually useful because people keep complaining about this
< promag>
wumpus: you don't know at what sequence corresponds a getrawmempool response
< wumpus>
no, indeed you don't
< shuckc>
I cannot know for sure if the txn hash broadcasts are adds or block removals, as they don't specify, and I can't for sure know how it lines up with the results of getrawmempool
< jnewbery>
for reliability, ZMQ can log seq_num:txid to file every time a notification is sent. If a client detects a missing seq_num, you consult the log and query the mempool rpc for that file
< wumpus>
zmq logging to file? taht sounds really at odd with low-latency
< jnewbery>
*for that txid
< luke-jr>
wumpus: mkfifo ☺
< wumpus>
luke-jr: fifo is one to one, not one to many
< luke-jr>
not sure why ZMQ is involved at that point lol
< luke-jr>
true
< wumpus>
luke-jr: if only UNIX had a one to many notification mechanism
< wumpus>
except for mail
< luke-jr>
dbus?
< sipa>
wumpus: oh you can have many writers and many readers for one fifo just fine ;) and no guarantee which write goes to which read
< aj>
wumpus: wall(1) :)
< * luke-jr>
is not sure dbus actually has this
< luke-jr>
aj: lol
< wumpus>
no, dbus doesn't have it either
< wumpus>
dbus is one to one, it has no realible multi consumer broadcase
< luke-jr>
I suppose you could just use a tmpfs file
< wumpus>
it's a difficult issue in general
< wumpus>
because some consumer might always be lagging
< luke-jr>
and punch holes at the start as desired
< wumpus>
this can potentially result in infinitely much storage needed
< luke-jr>
or store to disk and let Linux's buffers handle it
< wumpus>
rabbitmq is pretty good if you really need this
< instagibbs>
Well aside from fixing infinite buffer problems, I think it'd be good to improve where we can. When there's a failure there's always the fallback of getrawmempool for example
< jnewbery>
wumpus: zmq already logs (if -logging=zmq is enabled). It just doesn't log the seq num, so it's not easy for a client to tell which messages were dropped
< instagibbs>
I was joking that you could also do minisketch for set reconciliation of mempool views
< sipa>
haha
< luke-jr>
jnewbery: I'm not sure we want to encourage software to parse debug.log …
< instagibbs>
zmq to keep difference small ;)
< wumpus>
jnewbery: I don't think clients can ever know what message is dropped; usually, missing a sequnence number means having to resyncronize in some way (e.g.e query the entire mempool)
< wumpus>
luke-jr: exactly
< wumpus>
I don't think 'parse the log' is a good option, though it serves one-to-many notification perfectly
< wumpus>
mq is essentially a log
< wumpus>
until your disk is full
< luke-jr>
a dedicated, well-defined-format log might be okay
< wumpus>
it's also a high latency option but that might not matter
< luke-jr>
but something needs to do hole-punching to clean it up before disk fills
< luke-jr>
wumpus: why is it high latency?
< wumpus>
yes, but if you do tha, some clients might miss messages
< sipa>
luke-jr: bitcoind will already shut down when disk is full
< sipa>
:)
< luke-jr>
depends on who does it
< wumpus>
unless they tell you they read up until that far
< luke-jr>
sipa: yes, but you don't want that
< wumpus>
luke-jr: because disk/block devices are slow, compared to networking, latency wise
< luke-jr>
wumpus: Linux at least has buffers
< wumpus>
even with that
< luke-jr>
:/
< luke-jr>
the write/read won't even need to hit disk
< * luke-jr>
wonders if you can tell Linux to never flush to disk unless it has to
< luke-jr>
per-device*
< luke-jr>
per-file would also be nice :P
< wumpus>
yes, there's an option for that afaik, but it also means in case of power loss...
< jnewbery>
shuckc: it sounded like you were going to mention more issues. Was there anything else?
< wumpus>
reliable delivery of messages to multiple consumers is a difficult topic
< sipa>
we need a blockchain
< wumpus>
sipa: :-)
< instagibbs>
so i think the biggest oustanding issue(if evictions are announced and we're ok with drops once i na while) is being able to line up getrawmempool results with the notifications
< wumpus>
yes, at least the blockchain always allows going back i time... well unless pruning
< wumpus>
pruning is kind of the 'what if not all consumers have seen this yet' problem
< shuckc>
the sequence number on the response to getrawmempool
< shuckc>
obviously has backward compatibility concerns, and other suggestions?
< sipa>
shuckc: hmm?
< luke-jr>
wumpus: there is an option for that? what? :o
< instagibbs>
sipa, he wants to know where the mempool "snapshot" came from
< sipa>
does getrawmempool report a sequence number now?
< instagibbs>
no
< wumpus>
it doesn't
< shuckc>
no, I'm suggeting it should
< sipa>
and adding one would help?
< shuckc>
yes
< sipa>
oh, ok
< instagibbs>
so you don't know if the getrawmempool result is stale or from "future" wrt zmq reports
< sipa>
what would be the reason not to?
< shuckc>
because you can't tell which delta(s) have already been added
< wumpus>
I guess it could prove that there were no updates in between
< sipa>
adding new fields has no backward compatibility concerns
< instagibbs>
sipa, I think you'd have to add *all* the zmq notification seq numbers
< sipa>
ah
< instagibbs>
well it's an array result ;
< instagibbs>
;)
< promag>
can bnly be added if verbose=true in getrawmempool
< instagibbs>
but like I said I think optional arg -> json object
< wumpus>
I wonder why every zmq message has its own sequence number
< wumpus>
couldn't it be just one increasing atomic?
< shuckc>
unless you have one single notifier that you use for all the messages you need to sync with
< instagibbs>
wumpus, it's a local member of hte notification, for whatever reason
< promag>
wumpus: a client knows if he missed anything
< wumpus>
promag: oh, true
< shuckc>
it's only a mess if need to track lots of notifiers
< wumpus>
yes, makes sense, a client is generally interested in only a subset of message kinds
< instagibbs>
shuckc, shouldn't be too bad, you just grab the one you care about
< wumpus>
so a global sequence number would be useless
< promag>
wumpus: not really, that number should be exposed in both rpc response and zmq message
< wumpus>
in any case getmempool would only need the mempool related numbers...
< promag>
but I'm not fond of that..
< wumpus>
it seems like some kind of layer violation
< wumpus>
having RPC query ZMQ
< promag>
because as a client, you need to use rpc AND zmq
< wumpus>
yes
< instagibbs>
otherwise you need to somehow ask for unique snapshot
< promag>
hence my draft PR
< instagibbs>
of the mempool
< instagibbs>
but yes, it's a light violation at least
< phantomcircuit>
shuckc, zmq can and will silently drop messages, unless you have sequence numbers in the application layer you cannot detect that
< shuckc>
My suggestion includes connectblock and disconnect block notifications on the same new channel, because they allow you to keep your local mempool up to date and equally you need to know where in the stream of deltas they arrived
< wumpus>
of course, that could be worked around by having both RPC and ZMQ query another source for sequence numbers
< jnewbery>
how about if the mempool itself had a seq number that is incremented on every add/remove?
< shuckc>
with promags suggestion, bitcoind has to keep state/buffer for each consumer, the zmq model makes it state-less, and promag you also need to poll for new messages which is something of a step backwatfs
< wumpus>
I didn't understand it like that
< promag>
note that in my PR, the "stream" will be upper bounded in size, so no OOM concerns
< luke-jr>
shuckc: not if he adds longpolling
< wumpus>
the mempool itself would keep the seq number
< wumpus>
not per consumer
< promag>
shuckc: ill add long poll
< wumpus>
it's kind of strange if different consumers have differnt seq ids
< shuckc>
do any other commands use long polling?
< wumpus>
because this is global state we're exposing
< luke-jr>
GBT
< phantomcircuit>
wumpus, if zmq is using a global sequence number for all messages, i'd suggest just adding that to rpc as a header or something
< promag>
shuckc: yes
< instagibbs>
getblocktemplate <--- GBT
< promag>
I don't like longpoll that much tbh, at least in json rpc
< promag>
especially because of libevent, timeouts etcetc
< promag>
but "poll and wait up to n secs" if fine imo
< wumpus>
I don't think adding a different notification mechanism will really solve the 'clients could stop consuming and keep behind' problem
< luke-jr>
promag: same thing? :P
< wumpus>
it would mean accumulating in memory in that case?
< promag>
luke-jr: n secs < timeoud (:
< promag>
wumpus: yes
< fjahr>
I guess I am the hammer that sees nails everywhere, but how about a hash (muhash) for the mempool states instead of a sequence number? but not sure i grasp the problem completely yet...
< promag>
but thats's fine
< luke-jr>
fjahr: then you need to log the hash..
< promag>
if the client doensn't pull the stream, the stream will be terminated
< wumpus>
no, I don't think that's fine, if there's no limit a client could forget to connect and fill your memory entirely
< instagibbs>
fjahr, even more ridiculous than my minisketch idea ;P
< wumpus>
promag: that's just another "lose notifications" then
< fjahr>
hehe
< luke-jr>
wumpus: it's one thing to begin dropping stuff after N minutes of downtime; another to lose them randomly as a normal event
< wumpus>
reliable notification is really a non-trivial issue :)
< promag>
wumpus: no, the stream will be terminated, the client starts over and gets a fresh stream
< wumpus>
in any case, it's time
< aj>
instagibbs: (minisketch is a great idea!)
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jul 9 20:02:11 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< luke-jr>
promag: maybe this should be part of getrawmempool?
< instagibbs>
aj, just need the tool in -cli, I think :D
< shuckc>
if the longpoll machinery all works, then I don't have any reason why not to use it. So long as it can be sequenced with repect to the getrawmempool calls and covers all the removals/additions cases
< promag>
luke-jr: meh, let's not overload, at least for the momemnt
< wumpus>
it has the same problems I think
< luke-jr>
promag: well, they need to interact..
< instagibbs>
shuckc, I think the idea would be you'd replace getrawmempool entirely with this new call
< wumpus>
it's another method to receive notifications but doesn't avoid any of the problems, the HTTP buffer can also be full
< promag>
shuckc: you won't use getrawmempool anymore
< luke-jr>
wumpus: forever buffer vs limited buffer vs no buffer
< shuckc>
I kind of like that. getrawmempool could give the snapshot and then if you've got longpool flag then it continues to stream modifications
< wumpus>
luke-jr: zmq has a limited buffer, like anything
< promag>
wumpus: so you don't worry about http response size etc
< luke-jr>
promag: s/stream id/last update id/
< wumpus>
what if it exceeds the number of entries then?
< promag>
the concept is pretty much like postgres write-ahead-log and replication slots
< luke-jr>
promag: and for timeout, the client can just disconnect?
< luke-jr>
wumpus: then you need to get the entire mempool and start over
< promag>
luke-jr: no, stream id - you can have multiple clients, each with their own stream
< luke-jr>
(which is another reason to overload getrawmempool?)
< wumpus>
exactly as with zmq notifications if you miss a sequence number
< luke-jr>
promag: they shouldn't have separate streams
< luke-jr>
wumpus: except you're less likely to miss one
< wumpus>
I really don't see how a different notificatoin mechanism solves the underlying problems, but ok, if you belive so, go ahead
< wumpus>
'less likely' is no guarantee!
< promag>
need to afk, please view my PR code, it's very small/simple
< luke-jr>
wumpus: it doesn't need to be a guarantee to work well
< wumpus>
if losing a notification is a critical issue, a smaller chance of it is better but still a critical problem
< luke-jr>
it's not critical, just something you'd prefer to avoid
< instagibbs>
hopefully not critical anywhere
< shuckc>
I'll review it promag and see if it covers all our uses
< wumpus>
how do you mean 'work well' then, not lose money for *most* users?
< luke-jr>
wumpus: it's an optimisation
< luke-jr>
you *could* always just getrawmempool constantly, but it'd be slow
< wumpus>
you need to setup your client code to handle missing notifications in any case
< luke-jr>
yes, and hopefully it won't happen often
< wumpus>
you can increase the zmq buffer size for that
< instagibbs>
miss message -> patch with getrawmempool-like response
< instagibbs>
keep that infrequent, it's a win
< wumpus>
people have been discussing this since 2012 or so since zmq notifications were added
< instagibbs>
and yet, we don't have eviction notifications at all :P
< wumpus>
I wish we never did fwiw
< instagibbs>
understood though im sure it's tiring
< luke-jr>
wumpus: admittedly I haven't used ZMQ since these problems, but it didn't work for me
< wumpus>
it's only been like this every time
< wumpus>
ZMQ IS UNRELIABLE
< wumpus>
ITS USELESS
< wumpus>
yeshh
< wumpus>
like let's just remove it
< aj>
could convert mempool entry's nTime to a monotonic clock, and use that as a sequence? "getrawmempool -- but only entries added since time X" maybe
< instagibbs>
well, it's being used in the wild, SFYL
< instagibbs>
LND uses it too
< instagibbs>
(don't know why exactly, just does)
< wumpus>
instagibbs: I know
< promag>
wumpus: too late, sorry for that honestly X)
< wumpus>
instagibbs: I know it's actually used by peple in production, but the discussions are always the same
< wumpus>
I think it was like 'zmq is even used by high frequency traders and such it's probably useful for us'
< cfields>
sipa: I suppose the old gcc issue is that the propagate_on_container_* aren't respected ?
< sipa>
cfields: possibly
< sipa>
but it looks like that
< wumpus>
every time anyone mentions zmq it's: but it's unreliable!!! which is true, it's optimized for low latency, not reliability, all other mq products that historically were optimized for reliability, zmq intentionally made this trade-off
< jnewbery>
In general, I don't think we should add yet another notification system to work around a supposed shortcoming in ZMQ. Then we just have an additional notification system to maintain
< wumpus>
one other common reliable mq notification system nowadays is Apache Kafka
< jnewbery>
if ZMQ is really unfit for purpose it should be removed and replaced
< wumpus>
I agree
< instagibbs>
If you waved away the getrawmempool thing, I think it's probably fine enough
< instagibbs>
block notifications are very cheap to "heal", 300MB mempool is not
< cfields>
sipa: from libstdc++ 4.8 release docs "23.2.1General container requirementsPartialOnly vector and forward_list meet the requirements relating to allocator use and propagation."
< sipa>
cfields: ah
< cfields>
sipa: if it makes you feel any better, looks like support for all standard containers didn't exist until 6.x.
< instagibbs>
jnewbery, so I think you'd still need to make a layer violation(or replace getrawmempool with set reconciliation protocol) to smoothly use zmq, even with logging backing. I think that's two complementary improvements.
< instagibbs>
if you're going to have to use RPC, might as well use RPC?
< wumpus>
instagibbs: I guess it could be cheaper by using some binary format for the mempool; but in practice, losing messages should be rae, when I still used zmq for some notificaiton system I don't remember ever losing a message, just make sure that's a thread in the client that keeps reading
< sipa>
cfields: seems i may just need to give up on the no-propagate-on-copy, and add a big disclaimer
< cfields>
sipa: that's really a shame. I remember spending a few days banging my head against the wall trying to make scoped_allocator_adaptor do something interesting. I now see that it was probably just quietly broken under the hood the whole time.
< wumpus>
in any case, adding a sequence number to getrawmempool to that the full dump can be reconciled with updates makes sense
< wumpus>
independent of the notification system
< instagibbs>
wumpus, yeah, just saying aside from mempool sync protocol I think we need to tell the consumer where raw mempool responses work
< sipa>
cfields: also now i remember the reason why i made it inherit from std::allocator
< sipa>
old libstdc++ doesn't implement allocator_traits, so you need all dozen trivial functions and types on it
< cfields>
oh, heh, yeah, that'd be a problem.
< sipa>
it's just inconvenient
< wumpus>
it's much more practical to have a system that can handle (but generally doesn't need to handle) going out of sync than something that can guarantee 100% delivery, so if adding a update sequence # to the mempool solves that that'd be preferable imo
< cfields>
sipa: when I said "you're not supposed to inherit from std::allocator in c++11", I just meant that IIRC you're supposed to lean on allocator_traits instead. So yeah, without that, I see why you just inherited.
< cfields>
sipa: but given that support is missing from the containers themselves, I doubt that change will change anything for the better :(
< cfields>
sipa: also, nit, isn't std::allocator guaranteed stateless? Can't you just call its static members?
< cfields>
nm, obviously not.
< bitcoin-git>
[bitcoin] JeremyRubin opened pull request #19478: Kill mapLinks data structure in Mempool (master...epoch-mempool-clean-split-3) https://github.com/bitcoin/bitcoin/pull/19478
< cfields>
llvm's lld (in master, not released) is complete enough as of this week to link a working bitcoind for macos from linux, without having to resort to building apple's custom ld64 :)
< cfields>
jonatack: thanks for the motivation to build llvm today, I really wanted to test ^^
< cfields>
No idea how well at works, but "./bitcoind --help" works, at least.
< promag>
cfields: nice, builds qt too?
< cfields>
promag: heh, no clue. I didn't want to end up deep in qt link issues...
< cfields>
I should though. This work is active on lld right now. Good time for us to be reporting issues/patches.
< cfields>
promag: yeah, it doesn't know how to find .tbd frameworks yet, so no qt.
< cfields>
I'll check to see if that _should_ be working yet and report it upstream if so.
< phantomcircuit>
wumpus, you can just go to the zmq docs and see that it's not reliable, it's all about "implementing patterns" to achieve some kind of reliability
< phantomcircuit>
i've used it before but only with the knowledge that it's basically udp