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

Make DoubleSpendProof checking a little bit smarter. #237

Open
tomFlowee opened this issue Mar 9, 2024 · 5 comments
Open

Make DoubleSpendProof checking a little bit smarter. #237

tomFlowee opened this issue Mar 9, 2024 · 5 comments
Labels
suggestion New features to think about

Comments

@tomFlowee
Copy link

tomFlowee commented Mar 9, 2024

The basis of double spend proofs is that they are only really relevant for around 10 seconds since first seen. After that the client shouldn't really care about them anymore and if a user sends out a double spend, the chance of it getting confirmed is lim(0).

I'd be a fan of simply auto-removing a dsp subscription after 30 seconds for that reason. But that is not this report, this report is about something different.

Knowing the time-limit, a hole we have today may be relatively easy to close.

The hole is that if a thief were to send not exactly 1 transaction, but instead 2 (or more transactions) chained in less than a second to the network, then they could double spend the parent transaction to similarly make the actually paying transaction get orphaned.
And the bchn-rpc method would not notice, and similarly the fulcrum API would not notice. (please correct me if that is not true!, and please also update the docs if so).

Fulcrum could quite trivially fetch the relevant transaction the remote user asks for and check for parents. Especially if it checks the timestamps of first-seen in the mempool, it will be able to limit the chain quite severely.

All transactions that are inputs can recursively be monitored, but you don't need to monitor confirmed ones and you don't need to monitor anything that has been added to the mempool already more than 30 seconds ago. Not sure what the practical number of transactions-to-monitor will be, but I expect it to be well below than 100. And naturally only for about 30 seconds, they start dropping off very quickly.

While the actual number of to-monitor transactions is on average a little over 1.

Please consider adding this functionality to Fulcrum so wallets can rely on double-spend-proofs in real-world usecases.

As an aside, this was always seen as a blocker for big wallets supporting DSPs, and when those wallets support it and it is on the market then we can add p2sh support to the original dsproofs. That schedule is kind of out-of-whack with this 3 year wait. Not sure if we can just add the p2sh support without waiting for actual app-devs feedback and hope it will actually work for them even though they may not have tested it yet... Would be nice to have this feature so we can just move onto the testing and then the p2sh usecases.

@cculianu
Copy link
Owner

cculianu commented Mar 9, 2024

Yes, I am aware of this issue. It's insufficient to know that a txn has no dsproof, you need to check all the parents. And you need to ensure all ancestor txns are not p2sh or otherwise "ineligible" for dsproofs, on top of that .. to really be sure.

I started some work on this in a branch and have an untested implementation of this -- but it's quite a bit of work to get the 100% correct solution in place, and to test it -- since you basically need to maintain state for a child and its parents and worry about it and there are subtle corner cases. Also when parents confirm but children remain alive then you need to "realize" this and now a child may end up in a different dsproof state as a result of confirmations happening (typically its dsproof status is either the same or more "in the clear" than before). All of this needs to be handled for a correct solution and doing it half-assed is worse than not doing it at all.. given the implications.

@cculianu cculianu added the suggestion New features to think about label Mar 9, 2024
@tomFlowee
Copy link
Author

Not sure what the problems are you are seeing, I think it's pretty simple.

Corner cases don't really happen, you get asked about a tx, you are 100% certain that that tx is in the mempool and all its ancestors are in the mempool or confirmed. So it is a job of simply walking the tree.

(typically its dsproof status is either the same or more "in the clear" than before).

Ehm, I don't think I follow this. There is no "more in the clear". A transaction (and all its combined ancestors) either are free from DSProofs or they are not. Don't over-complicate things!

Sure, you need to maintain state, but consider the following class I just mocked up. It does ALL the needed cornercases and while it doesn't propagate the full dsproof details up, you can add that pretty easy. It is conceptually complete other than that, naturally the comments are to be filled in.

DSTxState.h.txt

@cculianu
Copy link
Owner

cculianu commented Mar 10, 2024

Not sure what the problems are you are seeing, I think it's pretty simple.

Well then go ahead and fork Fulcrum and implement it, if it's so simple. Have fun.

you are 100% certain that that tx is in the mempool and all its ancestors are in the mempool or confirmed.

Maintaining this invariant is messy because Fulcrum's modeling of the mempool is not designed to keep track of this. Due to how Fulcrum models the mempool (hint: it doesn't model it like Core does at all -- because it doesn't typically care about parent/child relationships at all).. it's messy.

There is no "more in the clear". A transaction (and all its combined ancestors) either are free from DSProofs or they are not. Don't over-complicate things!

Because a transaction can be in a state where some of its ancestors are not eligible for dsproofs -- so it should be flagged as "untrusted".. and then later its ancestors confirm and thus it has no ancestors that are no longer eligible -- it needs to migrate to the state of "trusted".

Don't over-complicate things!

Jesus christ. No comment. Stop being so preachy.

It does ALL the needed cornercases and while it doesn't propagate the full dsproof details up, you can add that pretty easy. It is conceptually complete other than that, naturally the comments are to be filled in.

It doesn't "do" jack because it's not integrated into Fulcrum's mempool code. The devil is in the details here.

@tomFlowee
Copy link
Author

Maintaining this invariant is messy because Fulcrum's modeling of the mempool is not designed to keep track of this.

The limit of only a short time relevance of the dsproof means you don't need to use the mempool.

Because a transaction can be in a state where some of its ancestors are not eligible for dsproofs -- so it should be flagged as "untrusted"

The best thing to do is to return with a simple error message: "this transaction is not eligible for dsproof protection" and cancel the subscription.
That is what I mean with don't overcomplicate things. The spec very clearly states some stuff isn't supported. Fulcrum is perfectly fine in following its lead and not supporting difficult corner cases.

Stop being so preachy.

I apologize if it comes across like that. I'm trying to align our understanding of the problem domain. As Jonas nicely pointed out on Telegram this morning (CET) you are solving a different problem than what people want Fulcrum to solve. Not being preachy, but I'd like to somehow get through that the problem you are trying to solve doesn't need solving.

Jonas wrote:

I think you and @cculianu might be talking past each other here and discussing different (but related) issues.

What I think @tomFlowee is asking:
Given a chain of "normal" unconfirmed p2pkh transactions (A, B and C) where a user subscribes to dsp for C, and A or B is double-spent, does the user get a notification for C being double spent?

The issue I think @cculianu discusses:
If A or B is a p2sh the user has no way of knowing that C will not get a DSP if A or B is double spent.

And he's right. I think that is the core difference. So my answer to you solving this issue of unsupported DSProofs is simple; don't try. Just report that the transaction is not eligible for proof-protection. Let the ecosystem work with the known limitations and find a different solution for this problem.

ps. noticed a constructor issue in the cpp file, here is an updated one:
DSTxState-2.h.txt

@tomFlowee
Copy link
Author

I want to add that the main reason I wrote the code is because it shows the requirements very well.

The signals:

signals:
    void finished();
    void failed();
    void proofFound();

are the interaction contract that I think make most sense, and people that would use this API seem to agree. I asked.

  • finished() is emitted after around 30 seconds, which concludes the subscription. No DSP found. Most often chosen path.
  • failed() is emitted very shortly after subscribe started if the transaction is not eligible for dsproof protection. Letting the merchant or software know that this one isn't protected and the payee needs to wait for a confirmation.
    The subscription is also concluded instantly.
  • proofFound() is emitted the moment any of the transactions in the chain have a double spend proof.
    The subscription is also concluded instantly.

This API is the reason the problem is made easy, nothing more than that. The requirements are made simpler, so the problem is simpler to solve.

Thanks for reading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion New features to think about
Projects
None yet
Development

No branches or pull requests

2 participants