adridadou / eth-propeller-core

Core library for Ethereum propeller
Apache License 2.0
26 stars 16 forks source link

handle chain reorg while playing blocks #36

Open adridadou opened 5 years ago

adridadou commented 5 years ago

thank you @ESchouten for your PR. Your change on how to handle reading the blocks made me think of another improvement that we should do.

Right now, the only thing we do is keeping track of the latest block number we've read and load / play the difference between the latest played and the latest available.

But here is the issue, in case of a re-org, we won't be playing new blocks that we've never seen.

So here is a proposal to handle that.

Instead of keeping the latest block number, we keep the 1000 latest block hash (this number should be configurable)

Now every time we check if there are new blocks, we do the following:

How to maintain the list? Because the order there is not important, it's just a way of saying what we've already seen or not, keeping it in no particular order but limit the size should be enough.

Having this list big enough should prevent any issues if we have more than 2 competing chains. Also, because we only keep the hashes, it doesn't take too much memory so keeping 1000 or even more shouldn't be an issue.

I'd love to get someone's feedback before implementing that. Maybe some improvement ideas

ESchouten commented 5 years ago

It's indeed very important to think about handling chain re-orgs.

The main problem I can think of handling these is; How do applications implementing Propeller process these reverts in their local (SQL) database, that is established from changes and event it has received from the blockchain? And what consequences does this have for local business logic constraints?

One way to prevent this would be to wait processing blocks until they have an X amount of confirmations, and let the remote node (e.g. Geth) handle the re-org. After these block confirmations we could assume the permanence of the block and process it.

A major downside of this would be the extremely slow, for some applications unworkable wait times until a transaction is processed.

adridadou commented 5 years ago

This is also something I had in mind, you should be able to define how many confirmations you want until you get an answer. It is slow but sometimes this is ok.

I need to think about the confirmation issue a bit more but I think more and more that it should be part of the API. Something like being able to specify in the way you listen to the result how many confirmations you want.

something like myContract.myCall(...).confirmations(numberOfConfirmations) and this future result will resolve once the number of confirmations has been reached. And by default, .confirmations() should be .confirmations(0) which is today's behavior.

WDYT of this API?

Maybe I should say that this idea is only for RPC backend. I think that EthereumJ already replays if there is a re-org and I don't think it makes sense to do that there.

Another thing I'd like to do at some point is implementing a Pantheon's backend since I am not sure how much ethereumJ works for public chains (non test ones) and Pantheon is more supported and developped.

So there is no reverts issues with RPC. The idea here is more to make sure you don't miss an information / event because of a re-org.

ESchouten commented 5 years ago

Implementing the confirmations into the call itself is a possibility, another one would be adding it to the EthereumConfig class which is used to configure the EthereumFacade. All non-constant calls would need the confirmations before being processed (unless stated otherwise, e.g. in a call configuration you just pointed out).

I do think we should create an overridable or configurable callback that gets called when such a re-org is done, so developers could decide what to do/execute internally when such a re-org is done and transactions could be reverted.

I might be overestimating the risk of reverted transactions, since with a re-org the transactions will be put in the pool again, and probably be added to the blockchain, but it would be nice to be able to validate this.

adridadou commented 5 years ago

so I prefer to let the confirmation number being set on each call rather than a config (but we could have a config to set the default one for ex). The reason being that I can easily see situations where you don't care about confirmations but situations when you do.

Regarding a trigger if there is a re-org, I'm wondering how much does it make sense. Let's see what we're trying to figure out here. My goal here si to make sure we don't miss any events for a contract. That's basically the only reason I think that it makes sense. If you're afraid that a re-org has screwed up your cached version of a smart contract, I feel like any block, reorg or not, can do that. A reverted tx is not really an issue IMO and if it is, you should mitigate the risks with a configured confirmation.

Another cool way to handle the confirmation issue is the following:

Instead of returning future, it returns a Stream (JavaRx). Each stream passes the following. the result + the number of confirmation since the result. In the confirmation you define the max number of confirmation you need to wait for. This way, you can simply say when you think the data is valid or not and you can configure that in one place. WDYT ?

ESchouten commented 5 years ago

I can agree with setting confirmation time per call, downside would be; this confirmation time will be needed to set in every instance of the codebase where this function is being called. While a custom wrapper class for the contract interface might fix this, could there be a solution to configure this per function e.g. in the contract interface?

I can see not needing a trigger when a chain re-org is done. Maybe I should see the blockchain more as a queryable database instead of an event register, and pull live state data on-the-go. That would solve the state revert issue I had.

Regarding the implementation of the confirmation amount, I think the stream solution could be a good one.

kivanov82 commented 5 years ago

Very nice improvement, and Stream per call is the best solution I'd say. I personally have all 3 cases where no/1/x confirmations needed. And reacting on each block would' be handy, similar to web3 stuff.

How the 'stack' of block hashes will be filled initially? I think it shouldn't propagate back in the past, as it would be confusing (4+ hours)?

adridadou commented 5 years ago

So the startup behavior is a good question. I see two different possible behaviors here:

  1. We only load the 1000 last block hashes but we don't do anything with them
  2. We load the blocks + we replay them

I guess each use case has pros and cons. We could make the behavior a config property.

Regarding the stream, I think I have an idea but I need to play with it a bit more.

As you know, right now we have EthCall. We could imagine a new type called ... EthResult (let me know if you have a better idea)

class EthResult<T> {
  T result;
  int confirmations;
}

class EthCall<T> {
   Flowable<EthResult<T>> result;
   EthHash transactionHash;
} 

This way, you could get the transaction results from the EthCall and play with the confirmation to filter to get the result you're expecting.

WDYT ?