< fanquake> I'm aware of some large companies that are using zmq extensively in production
< jeremyrubin> the ZMQ interface we have always struck me as a bit weird
< jeremyrubin> I guess if it works for people it works
< jeremyrubin> but I think phantomcircuit your intuition is correct that it might be used wrongly
< sipa> it'd be saner if it contained just txids/block hashes
< sipa> and didn't contain any actual data
< phantomcircuit> fanquake, then they are risking financial loss
< phantomcircuit> the zmq interface has zero reliability guarantees
< fanquake> let us know what you plan on replacing it with
< phantomcircuit> sipa, even then it's pointless, you need to poll to guarantee consistency, so why not just poll?
< phantomcircuit> fanquake, let me know whose using it so i can not use them
< sipa> phantomcircuit: depending on the application it's perfectly fine to poll infrequently, and also poll immediately when a zmq notification comes in
< phantomcircuit> sipa, the class of application for which that's ok is basically restricted to the websites displaying the mempool
< phantomcircuit> sipa, if the zmq interface was narrowed to be a single bit "you want to poll" that would be fine
< phantomcircuit> but also nobody would use that
< instagibbs> sipa, there are multiple subscriptions for just hashes, "sequence" notifier also lets you detect dropped messages and fallback to a poll
< sipa> right, that was added
< phantomcircuit> instagibbs, yes but that means you have a system where failing to do something results in a synchronization failure instead of a system where you're polling
< bitcoin-git> [bitcoin] jarolrod opened pull request #21414: doc: update macOS depends platform triplets (master...macOS-platform-triplets) https://github.com/bitcoin/bitcoin/pull/21414
< bitcoin-git> [bitcoin] fanquake opened pull request #21415: refactor: remove Optional & nullopt (master...remove_optional_wrapper) https://github.com/bitcoin/bitcoin/pull/21415
< wumpus> phantomcircuit: i don't want to remove zmq, i really like to have a notification mechanism that is not 'call out to a process'
< wumpus> phantomcircuit: that is the thing i dislike deeply so let's just agree to disagree
< phantomcircuit> wumpus, how about not sending actual data
< wumpus> *all* notification mechanisms have some degree of unreliability, there is no such thing as 100% reliable notification
< wumpus> the sequence numbers solve most concerns it pretty well imo and yes you *can* use it wrong
< wumpus> we had this discussion soo many times
< wumpus> i like having actual data this is good for low-latency default path
< wumpus> there are valid reasons for using different kinds of designs, different compromises
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #21408: doc: Explain how NACK commit_id works (master...2103-docNack) https://github.com/bitcoin/bitcoin/pull/21408
< vasild> "1 ACK" label :)
< jnewbery> wumpus: I agree. We know that people and companies use ZMQ so there'd need to be a much stronger reason to remove it than some other people don't like it. If you don't like ZMQ, don't use it.
< MarcoFalke> I don't think we've ever had issues with ACK spam
< MarcoFalke> Yes, there are issues where bot click the GitHub approve button
< MarcoFalke> But that doesn't influence ACKs in any way
< MarcoFalke> Currently it is impossible to find out whether a pull has at least one ACK without loading the page and going through all comments
< fanquake> Any relevant ACK will always have to be within the most recent few comment no?
< MarcoFalke> Sure, so you'll have to find the most recent commit and then read the comments from there
< jonatack> My impression was ACK quality > quantity (even CONTRIBUTING.md discusses weighing by merit)
< MarcoFalke> Of course the number of ACKs doesn't say anything about their quality
< jonatack> (or quality of feedback... I've invalidated 7 ACKs before to address review feedback)
< MarcoFalke> We have contributors complain that the pull request process (or what state a pull request is in) is not transparent, so I don't see why an attempt to make it more transparent is rejected so strongly
< MarcoFalke> bitcoinacks already does that, so the label is just bringing that to the "native GitHub" feel
< jonatack> The process can sometimes be confusing (what does this mean, what is the appropriate response, etc). It's a human one, after all.
< jonatack> I don't have an answer to it, though.
< promag> phantomcircuit: in doc/zmq.md "Therefore, subscribers should validate the received data since it may be out of date, incomplete or even invalid"
< vasild> MarcoFalke: I like the "1 ACK" label, would it be invalidated by a force-push? Should it be?
< promag> phantomcircuit: production code should not rely just on zmq, I'd expect people using pubsub zmq would know that
< vasild> "1 outdated ACK" label :)
< MarcoFalke> Yes, my idea was to only count the ACKs that are also counted in a merge commit
< promag> phantomcircuit: but its a great way to avoid frequent polling
< vasild> so you wrote some script to deduce if a PR has at least one ACK (on the latest topmost commit)?
< MarcoFalke> the script isn't written yet. It was an idea from yesterday
< vasild> you did that manually?
< MarcoFalke> yes, for two pull requests
< vasild> I think that is (would be) very useful
< vasild> maybe even (feature creep!) "2 ACKs", "3 ACKs", "many ACKs"
< promag> sorry guys, but what is the goal of those labels?
< MarcoFalke> (1) to see in conflicts how many ACKs they have (and how many acks would be invalidated)
< MarcoFalke> (2) to see if a pull request has $n ACKs and might be ready for merge
< promag> ty
< vasild> (3) (maybe that is just me) Sometimes I am in the mood of reviewing pristine PRs (no ACKs yet) and sometimes in the mood of helping with some PR that is on the brink of a merge and may need just one more ACK
< vasild> ... or sometimes PRs that have numbers that end in ...55 or commits that begin with fa...
< promag> MarcoFalke: how about having a gh project for that? there you would have a auto managed column with cards ordered by "maturity" or so
< MarcoFalke> I think the number of ACKs doesn't say how mature a pull is. It also depends on what code the pull is touching
< MarcoFalke> For example a test-only change with one ACK is likely more mature than a versionbits change with two ACKs
< MarcoFalke> s/is likely/may be/
< bitcoin-git> [bitcoin] laanwj pushed 13 commits to master: https://github.com/bitcoin/bitcoin/compare/63314b8211d7...767bb7d5c56b
< bitcoin-git> bitcoin/master a04aac4 Carl Dong: validation: Remove extraneous LoadGenesisBlock function prototype
< bitcoin-git> bitcoin/master d0de61b Carl Dong: miner: Pass in chainstate to BlockAssembler::CreateNewBlock
< bitcoin-git> bitcoin/master 46b7f29 Carl Dong: scripted-diff: Invoke CreateNewBlock with chainstate
< bitcoin-git> [bitcoin] laanwj merged pull request #21270: [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules (master...2020-09-libbitcoinruntime-v6) https://github.com/bitcoin/bitcoin/pull/21270
< bitcoin-git> [bitcoin] laanwj pushed 12 commits to master: https://github.com/bitcoin/bitcoin/compare/767bb7d5c56b...e828fc8f528d
< bitcoin-git> bitcoin/master d769b33 fanquake: build: only pass -optimized-tools to qt in debug mode
< bitcoin-git> bitcoin/master 3272e34 Hennadii Stepanov: build: Add xkbcommon 0.8.4
< bitcoin-git> bitcoin/master 06cd0da fanquake: build: qt 5.12.10
< bitcoin-git> [bitcoin] laanwj merged pull request #21376: depends: Qt 5.12.10 (master...qt_5_12_10_enhanced) https://github.com/bitcoin/bitcoin/pull/21376
< hebasto> \o/
< promag> MarcoFalke: right, I'm not saying maturity is # of acks, but there could be multiple columns for each sorting criteria
< promag> (#-of-updated-ack x sum-of-label-weigthts) / (1 + #-conflicts)
< promag> X)