Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

It's time (Enable fullrbf) #3867

Merged
merged 1 commit into from
Jul 17, 2023
Merged

It's time (Enable fullrbf) #3867

merged 1 commit into from
Jul 17, 2023

Conversation

softsimon
Copy link
Member

@softsimon softsimon commented Jun 15, 2023

AntPool, Binance Pool and Mara pool are among the mining pools now mining full RBF transactions just by looking for a while.

We catch these transactions when they get mined, but they show up as "added" skewing the audit template.

There are mostly benefits in enabling fullrbf to get the full picture of the mempool and not just throw "transaction not found".

Screenshot 2023-06-15 at 21 04 17 Screenshot 2023-06-15 at 21 04 56

@cla-bot cla-bot bot added the cla-signed label Jun 15, 2023
@softsimon softsimon marked this pull request as ready for review June 15, 2023 21:51
@softsimon softsimon requested a review from wiz June 15, 2023 21:51
@nymkappa
Copy link
Member

There are mostly benefits in enabling fullrbf to get the full picture of the mempool and not just throw "transaction not found".

mostly benefits. What are the tradeoffs? 👍🏻

@softsimon
Copy link
Member Author

There are mostly benefits in enabling fullrbf to get the full picture of the mempool and not just throw "transaction not found".

mostly benefits. What are the tradeoffs? 👍🏻

AFAIK that fullrbf transactions will get flagged as removed in some cases where non-fullrbf miners mine them (currently Foundry), but I made an issue to make these separately and you could argue that the mining pool is leaving money on the table here.

That's it as I know. Also that there might be some corner case transactions that will be viewable only on a non-fullrbf node.

Copy link
Member

@wiz wiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should yolo set mempoolfullrbf=1 without discussing more first... historically instead of forcing our decision on users, we tend to implement big decisions as user preferences, so users can decide if they want mempoolfullrbf=1 or mempoolfullrbf=0

As for the issue you've described auditing blocks, if we set mempoolfullrbf=1 then this change would be auditing all other mempoolfullrbf=0 mining pools using a mempoolfullrbf=1 block template, causing our auditing feature to become even less accurate - short term fixes like #3879 feel like a better workaround for now.

Let's discuss this more and come up with a proper spec to implement mempoolfullrbf=1 in mempool.space, and consider all the implications for our enterprise sponsors, integration partners, and users before rolling it out to production.

@wiz
Copy link
Member

wiz commented Jul 11, 2023

Okay, now that #3879 has been merged we can proceed with this.
It would also be nice to hear from @glozow @TheBlueMatt @jaybeddict1 before we set mempoolfullrbf=1

@mempool mempool deleted a comment from Weezytoughtem Jul 11, 2023
@TheBlueMatt
Copy link
Contributor

Wonder if there's a way to run both and give an rbf & not-rbf template, kinda seems like its bad either way - either rbf miners get marked as bad, or non-rbf miners do.

@jaybeddict1
Copy link

agree with Matt. not sure the overhead with that approach, but would give best of both worlds as pools adjust to the reality of fullrbf.

having said that, i think that mempoolfullrbf=1 should be enabled if the above is not feasible.

accurately reflecting the reality of an adversarial environment (fullrbf) is more important than having the audit score higher for pools that have yet to adapt. pools that elect to ignore it are harming themselves economically, which would now be reflected in the audit score. from our perspective, we are actively investigating enabling it, so the impact to our numbers would be short lived and explainable as it would be clear when we enable it.

@TheBlueMatt
Copy link
Contributor

This ignores the purpose of the mining score - pools can choose whatever policy they wish within reason, and the mining score should seek to match exactly what they are doing, providing an indicator that pools/miners are deviating from policy (ie due to censorship attacks), rather than them deciding to have a certain policy.

@jaybeddict1
Copy link

agree, and prefer your approach of running both if possible. mempool.space should strive to be as neutral as possible here as it risks confusing users. this also brings is inline with what Wiz noted above, albeit rather than offering it as a user preference it is just covering both sides so users are not forced to view from one perspective.

in my alternative approach i was viewing the audit score more from too general a perspective rather than the policy adhesion, which was incorrect.

would still like to understand the lift/overhead needed to run both options.

@junderw
Copy link
Sponsor Member

junderw commented Jul 11, 2023

Problem is: We can't know for certain the reason why a tx was included or not. One tx might look like an RBF replacement via fullrbf but it's actually a miner double spending their own tx to dupe a merchant who dumbly accepted zero conf.

Assuming absolute knowledge, the latter would be a huge ding to the miner's audit score... but we can't know, so we make compromises.

My point is: I think we should just keep the audit as-is and switch over to fullrbf once a certain threshold of miners are using it.

Maybe just add a disclaimer above the audit (ie. "Remember, audit scores are based on inclusion or exclusion of transactions only, There are times when a 0% health block is just an empty template due to finding the hash early, sometimes a 100% block becomes a 90% block because it changed a policy. This should only be alarming if the same pool is getting consistently <50% health scores etc. in which case further investigation may be warranted. However, witch hunting mining pools that are consistently in the 70~80% range is not productive and can just be the result of some policy change.")

@junderw
Copy link
Sponsor Member

junderw commented Jul 11, 2023

If we move forward with "make templates in 2 differing policies and compare notes" then what happens when another policy change appears before fullrbf is "decided" by the pools. Then we'll have 4 templates (each new policy will x2 the previous, assuming a simple boolean flag)... this is not tenable imo.

@junderw
Copy link
Sponsor Member

junderw commented Jul 11, 2023

If all pools start getting consistently low audit scores, and none of it is malicious, then that will most likely mean we need to change which policy to use.

Finding the one policy that is most generous to most of the pools is probably a better use of time in that situation.

Just my 2 cents.

@mononaut
Copy link
Contributor

I don't think making both fullrbf and non-fullrbf templates is worth the extra overhead - we'd need to add a third instance of Core to track the other mempool policy, and double all of the costs associated with calculating, storing & serving those templates.

With #3879 we no longer penalize miners for including or not including full-RBF related transactions anyway, regardless of which policy we run ourselves. There may be a small indirect penalty for excluding transactions which depend on other (non-)fullrbf txs, but I haven't actually seen this happen yet.

@mononaut
Copy link
Contributor

The other (imo more important) benefit of mempoolfullrbf=1 is that our nodes will get to see a strictly larger set of transactions - both the original non-rbf transactions and any fullrbf replacements - which lets us serve both versions to our users.

Screenshot 2023-07-12 at 8 52 09 AM

Right now we're completely blind to fullrbf transactions, which is worse both for users who want to see their fullrbf txs on mempool.space, and for users who may be depending on a non-rbf transaction and are unaware that it has already been replaced in some miners' mempools.

@TheBlueMatt
Copy link
Contributor

The audit score is borderline useless unless it reports 100 for every pool all the time....unless there's a deliberate issue - if it can't identify a single transaction being censored it should be rewritten to make it support that.

Reporting some arbitrary score measuring whether the miner's mempool happens to match mempool.space's mempool is not in any way an interesting or useful metric - these can diverge for all kinds of perfectly benign reasons. Specifically, a miner mining a double spend of equivalent fee may be an indication of some attack, but it certainly isn't necessarily the miner doing something bad. The same goes for full RBF.

The second you start fuzzing the boundary between the score getting a policy cludgel to "ding" miners doing something you don't like (but not censoring transactions for "Bad" reasons) it ceases to have any value for the original intent, and any value as a policy cludgel is orders of magnitude lower than as a true censorship detection flag (not score - binary flag).

@TheBlueMatt
Copy link
Contributor

Now, if it's simply impractical to run both, one question is how to rewrite it to capture both without running both bitcoinds - i think an alternative statement is if a block is "good" is "did all the transactions I had in the top 3MW of my mempool either get mined or conflicted". That way it doesn't matter which bitcoind is being run, all we care about is that there isn't something hanging around in the top of the mempool that for some (possibly very bad!) reason isn't being mined.

@glozow
Copy link

glozow commented Jul 12, 2023

The majority miner policy would be the most accurate one to use, though it seems the majority may be changing and there are multiple (and there may be multiple for the foreseeable future). If you expect 2 predominant policies, I agree the best approach in is to run both.

The other (imo more important) benefit of mempoolfullrbf=1 is that our nodes will get to see a strictly larger set of transactions - both the original non-rbf transactions and any fullrbf replacements - which lets us serve both versions to our users.
Right now we're completely blind to fullrbf transactions, which is worse both for users who want to see their fullrbf txs on mempool.space, and for users who may be depending on a non-rbf transaction and are unaware that it has already been replaced in some miners' mempools.

Generally I think it'd also be good to move to a "multiple versions of this transaction exist" instead of "RBF history" model in the displays.

You see more transactions if you have full rbf on, but you still only keep 1 in the mempool. Hopefully people are not using mempool.space to "see whether I got paid" or "see whether my payment got sent," for unconfirmed transactions, but displaying alternative versions could reduce surprises and more effectively teach the "it's an adversarial environment and your unconfirmed tx isn't final" lesson (if that's what you're going for).

@hunicus
Copy link
Contributor

hunicus commented Jul 17, 2023

We went through this thread together IRL...a central theme has to do with forcing a perspective. Whether for block audits or showing RBF events to users for a particular transaction—we don't want to force a perspective or purport a "right" way of doing policy.

For transactions, the flavor of RBF being used will be more apparent. Differentiation is already implemented with the shape of the transaction point in the RBF history diagram (circle vs square), but it is subtle, so we've improved it with colors and other visual elements so it's more clear to users when a replacement was done with a node's mempoolfullrbf policy or with BIP125 signalling.

mononaut implemented some such changes with #3957:

full-rbf-demo

.

This ignores the purpose of the mining score - pools can choose whatever policy they wish within reason, and the mining score should seek to match exactly what they are doing, providing an indicator that pools/miners are deviating from policy (ie due to censorship attacks), rather than them deciding to have a certain policy.

As mononaut mentioned earlier, #3879 excludes full-RBF transactions from the block audit and block health score altogether along with existing exceptions such as recently seen, marginal feerate, etc.

Here are block health scores before and after 3879:

Before 3879:

block-audit-before

.

After 3879:

block-audit-after

.

I think we should just keep the audit as-is and switch over to fullrbf once a certain threshold of miners are using it.

At this point >10% of hashpower is using mempoolfullrbf, and it's highly effective, so a threshold may already have been met.

Enabling the flag at this point will allow us to show users transaction info from the view of nodes running both flavors of policy before and after confirmation, and now it won't penalize miners either way either.

Copy link
Member

@wiz wiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it

@wiz wiz merged commit 924782a into master Jul 17, 2023
20 checks passed
@wiz wiz deleted the simon/enable-mempoolfullrbf branch July 17, 2023 12:48
@petertodd
Copy link

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants