cculianu / Fulcrum

A fast & nimble SPV Server for BCH, BTC, and LTC
Other
349 stars 79 forks source link

Make DoubleSpendProof checking a little bit smarter. #237

Open tomFlowee opened 8 months ago

tomFlowee commented 8 months ago

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 commented 8 months ago

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.

tomFlowee commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.

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.

tomFlowee commented 1 month ago

Recent discussions on the topic of dsproof gave the insight that the technical view of this concept may be different from what the market wants from this concept. So here a different perspective to understand the 'requirements' better.

Without dsproof the merchant can be someday being double spent with zero warning. It may turn out that the thief tried a dozen times already, every time they shop there, but there is zero communication about this to the merchant. The point of dsproof is then one of better assessing risk.

The currently operational dsproof protects in a very shallow set of cases. Only if all inputs are confirmed. This adds so little to the risk assessment that most don't actually implement this.

What this issue requests is to expand the risk-assessment parameters and aim to include most, if not all, fair use and honest payments to brick & mortar merchants. The user experience (for the merchant) is expected to instantly return with an initial risk assessment: This transaction you requested has more than N ancestors. This transaction is not protected. From the perspective of a merchant receiving payments for goods delivered, this is a red flag. I mean, think about it. How hard is it for wallets to avoid this situation? It is trivial since it actually takes a lot of work to GET into that situation. So from the point of view of the merchant that wants to protect his funds, it is perfect to mark that user that is doing something funny with that red flag. Making them wait for a confirmation.

The second part of the user experience is that the point of sale subscribes to the transaction they received and wants a notification if there is a double spend. And after 5 or 10 seconds we know from basic network topology that a double spend is practically impossible to get mined, we stop looking for double spends.

The effect, again from the merchant point of view, we are looking at is to make all normal day to day interactions "protected" transactions because that drops the risk of in real life double spends.


Now, there is a focus on somehow making dsproof cover everything. But this was never the goal. Normal usage of Bitcoin Cash doesn't involve you creating a 100 deep chain and then in the same 3 seconds paying a merchant. It doesn't include any p2sh in that chain of transactions. Normal wallets can avoid this trivially. So when abnormal transaction chains are created, and we can 100% correct detect those, that is enough.

cculianu commented 1 month ago

I see things exactly the same as you on this topic -- you just want 99.999% of normal transactions that are going to be normal wallets doing normal things covered. Anything outside of that like deep unconf. chains or funny p2sh unconf. parent txns, are ok to flag as low confidence/no confidence. It's all about confidence.

jimtendo commented 1 month ago

Hi all, just wanted to detail how we were (once upon a time) using DSPs for Hop.Cash (RIP). I'm not aware of any other real-world examples of DSP's in use, so thought it might be helpful to show how we were consuming them on the service side (and how other services might want to leverage them).

Hop.Cash had a few constraints on the use of DSPs:

  1. The satoshi amount provided must be under a set threshold to allow DSPs (I can't remember exact value, but I think it was < 1 BCH).
  2. The given transaction must be eligible (capable) of providing DSP protection (if not, we just made the user wait until a confirmation... or maybe two if it was a large amount being sent).

My memory is a little fuzzy here but, for the latter (eligibility), we didn't want to trace through the ancestors of the given transaction as I think this would've been expensive (and complex): we would've had to make Fulcrum blockchain.transaction.get calls sequentially for each ancestor and, given sufficient depth, it may've caused too much latency (request transaction ancestors -> wait for response -> get transactions ancestors -> wait for response -> etc). Instead, we used a custom patched version of Fulcrum (and possibly custom config'd node?) that added a passthrough to the bchn.getdsproofscore endpoint (for others who might read, this passthrough capability is now OOTB with Fulcrum via daemon.passthrough RPC endpoint, but I think you'd have to connect to your own BCHN instance with getdsproofscore method whitelisted.).

For transactions that were eligible for DSP's, I think all we did (which probably could've been improved) was:

  1. Broadcast the transaction
  2. Wait for a set amount of time (propagation delay) and then called bchn.getdsproofscore (if response is 1, all is good... otherwise, wait for confirmation).

I'm not sure on the feasibility of something like a getdsproofscore in Fulcrum and Jonas mentioned last night that the BCHN method might be too expensive to open up by default. But, if an endpoint similar to that is feasible, I think it would make it relatively simple/straightforward for PoS systems, Ecommerce stores, etc, to implement. The difficult/complex part, currently, really is working out whether a given tx is actually a candidate for DSPs, but I think Fulcrum already has the other RPCs that would be required (dsproof.get, dsproof.subscribe, etc).