Redemption flow actions #585

lukasz-zimnoch · 2020-10-22T12:01:10Z

Refs: #574
Depends on: #580
Depends on: keep-network/tbtc#740

This pull request expands the TBTC extension by adding two new actions specific to the deposit redemption flow.
Here is a brief summary of both actions:

ProvideRedemptionSignature action

Starts the monitoring on new RedemptionRequested event. That event is emitted either by requestRedemption or increaseRedemptionFee contract methods.
Stops the monitoring if GotRedemptionSignature or Redeemed event occurs for the observed deposit. This means either provideRedemptionSignature or provideRedemptionProof has been called by someone else.
Performs the provideRedemptionSignature action once the deposit is close to exceeding the redemption signature timeout.

ProvideRedemptionProof action

Starts the monitoring on new GotRedemptionSignature event. This event occurs once provideRedemptionSignature contract method is called.
Stops the monitoring if RedemptionRequested or Redeemed event occurs for the observed deposit. These events can be emitted by increaseRedemptionFee or provideRedemptionProof respectively.
Performs the increaseRedemptionFee action once the deposit is close to exceeding the redemption proof timeout. This part will be extended soon by adding the ability of submitting the redemption proof directy if the BTC transaction exists.

Added a stub of the provide redemption signature monitoring process. Also, implemented an events log which will be needed at later stage.

Implemented the main part of the redemption signature action.

pkg/chain/ethereum/ethereum.go

# Conflicts: # pkg/extensions/tbtc/tbtc.go

pkg/extensions/tbtc/events_log.go

pkg/extensions/tbtc/tbtc.go

pdyraga · 2020-10-23T10:06:15Z

pkg/extensions/tbtc/tbtc.go

+		}
+
+		return fmt.Errorf(
+			"could not find signature for digest: [%v]",


Worth confirming at some point if the digest is logged in a human-readable form this way.

Implemented almost all elements of the provide redemption proof action. Also, some code refactoring has been done by the way.

pkg/chain/ethereum/tbtc.go

pkg/extensions/tbtc/tbtc.go

pdyraga · 2020-10-26T07:30:15Z

pkg/extensions/tbtc/tbtc.go

+		if err != nil {
+			return nil, err
+		}
+


Hate to say that but I think we need to block this function here for some time (6-12 blocks) and check the chain before we signal the monitoring goroutine to stop. In case of a chain reorganizaion, we may get an event, stop monitoring, but the final chain state may not include the transaction with the redemption signature provided. In this case, our monitoring goroutine should act. I am not sure yet how it impacts the timeouts and exponential backoff for retries.

The same comment applies to all monitoringStopFn implementations.

If this suggestion does not lead to changing the code structure significantly - and I think it does not - I am fine with just leaving a TODO here and addressing this and all other monitoringStopFn implementations in a separate PR. Leaving the decision to you.

I'd vote for a separate PR. This one is already big enough.

Sure, let's just leave a TODO for now.

Handled in #591 but I'll this discussion open here until that PR is reviewed and merged.

pkg/extensions/tbtc/tbtc.go

pdyraga · 2020-10-26T07:39:51Z

pkg/extensions/tbtc/tbtc.go

+
+		redemptionRequestedEvents, err := t.chain.PastDepositRedemptionRequestedEvents(
+			depositAddress,
+			0, // TODO: Should be something better that 0


This could make all clients deployed against Infura unhappy. As far as I know, they allow to reach no more than 1000 blocks in the past and assuming each block takes 15sec on average to mine, this is just 250 minutes back.

If there is no workaround, without seriously sacrificing code quality, we are fine. Just raising a concern.

Unfortunately, the approach presented here is the safest and simplest one. We can cache incoming events as an alternative but given event duplications, chain reorgs and stuff, I want to avoid caching as it strongly complicates the code.

I had to think more about it and load entire context into my brain. More thoughts on that:

We need duplicate event filtering for monitoringStartFn. The monitoring we should go with could be a time cache with a relatively short expiration time informing about the fact we triggered action X for deposit Y. If we know there is a pending X:Y monitoring, we do not start a new one. It is not yet clear to me what the expiration time should be but I guess the time needed to mine 6-12 blocks should be enough.

I think this approach is better than e.g. filtering out events by digest or any other property, as it's aligned with the approach proposed currently for actFn, and this way, we do not need to care about forks and two different events for the same notification in the competing branches.

We should not wait for any confirmations in monitoringStartFn as there is a chance we'll lose events from monitoringStopFn. If we are not already monitoring X:Y, let's start a goroutine immediately.

We should wait for 6-12 block confirmations in monitoringStopFn before we decide to stop the monitoring routine.

I like we are stateless actFn and we don't care about the particular event instance but we are oriented in the context of deposit and monitoring of a certain state transition.

Once we act in actFn we should wait for 6-12 blocks and check the state of the deposit. We need to make sure our transaction has enough confirmations.

I've added the link to this discussion to the TODOs section where I listed all the things related to events deduplication, reorgs etc.

Leaving this discussion open until all follow-up PRs addressing those issues are reviewed and merged.

pkg/extensions/tbtc/tbtc.go

lukasz-zimnoch · 2020-10-27T14:02:15Z

@pdyraga added the unit tests and addressed all comments! I'm ready for the next look.

pdyraga · 2020-10-28T10:00:41Z

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/ipfs/go-log v1.0.4
 	github.com/keep-network/keep-common v1.2.1-0.20201020114759-19c123cbd4f4
 	github.com/keep-network/keep-core v1.3.0
-	github.com/keep-network/tbtc v1.1.1-0.20201020115551-5f9077c74826
+	github.com/keep-network/tbtc v1.1.1-0.20201026093513-cb9246987718


We still need to implement a solution in keep-common for a workaround in keep-network/tbtc#740 before merging this PR, right? Otherwise, we would end up with a situation where keep-ecdsa master would not build against tbtc master.

Nope, we don't have to do anything in keep-common.

Don't we need generators for eventlog? From the description of the referenced PR:

Added a TBTCSystemEventLog allowing to access past events emitted by the TBTCSystem contract. This is a temporary workaround because contract bindings generated by the keep-common generator don't support this feature yet.

We just need the abi file which is already generated and tracked by the VCS.

We do need to implement a change to the generator in keep-common so that it generates TBTCSystemEventLog, correct?

Let me put it this way:

We build tBTC go package from scratch:

we remove all generated files from pkg/chain/ethereum/gen

we do go generate ./... there

we push a new version

Then we build keep-ecdsa:

we update a dependency to tbtc

we run go build

My understanding is that the compilation will fail because TBTCSystemEventLog will not be there.

In other words, we are depending on a file that should be generated but it's not generated.

Not quite. Eventually, we should extend the existing keep-common generator in that way it generates the Filter* methods straight into the TBTCSystem binding struct. TBTCSystemEventLog is just a temporary workaround created manually without using the generator. It is also tracked by VCS similarly as the regular bindings. So, when we build tBTC package from scratch we shouldn't delete anything. We should only run go generate ./... to capture possible contract changes.

Right. So just to be clear - I am absolutely fine with merging this PR having TBTCSystemEventLog created manually just like now but I do not want to release a new version before we have this file generated or having Filter* methods generated into TBTCSystem struct.

I'll leave this discussion open until the proper solution is implemented. I do not want to block this PR, though.

go.sum

pkg/chain/chain.go

pkg/chain/ethereum/connect.go

pdyraga · 2020-10-28T12:43:04Z

pkg/extensions/tbtc/tbtc.go

+		if err != nil {
+			return nil, err
+		}
+


Sure, let's just leave a TODO for now.

pdyraga · 2020-10-28T13:36:42Z

pkg/extensions/tbtc/tbtc.go

+
+		redemptionRequestedEvents, err := t.chain.PastDepositRedemptionRequestedEvents(
+			depositAddress,
+			0, // TODO: Should be something better that 0


I had to think more about it and load entire context into my brain. More thoughts on that:

We need duplicate event filtering for monitoringStartFn. The monitoring we should go with could be a time cache with a relatively short expiration time informing about the fact we triggered action X for deposit Y. If we know there is a pending X:Y monitoring, we do not start a new one. It is not yet clear to me what the expiration time should be but I guess the time needed to mine 6-12 blocks should be enough.

I think this approach is better than e.g. filtering out events by digest or any other property, as it's aligned with the approach proposed currently for actFn, and this way, we do not need to care about forks and two different events for the same notification in the competing branches.

We should not wait for any confirmations in monitoringStartFn as there is a chance we'll lose events from monitoringStopFn. If we are not already monitoring X:Y, let's start a goroutine immediately.

We should wait for 6-12 block confirmations in monitoringStopFn before we decide to stop the monitoring routine.

I like we are stateless actFn and we don't care about the particular event instance but we are oriented in the context of deposit and monitoring of a certain state transition.

Once we act in actFn we should wait for 6-12 blocks and check the state of the deposit. We need to make sure our transaction has enough confirmations.

pkg/chain/event.go

pkg/chain/local/bonded_ecdsa_keep_test.go

pdyraga · 2020-10-28T14:39:23Z

pkg/extensions/tbtc/tbtc.go

-const maxActAttempts = 3
+const (
+	maxActAttempts           = 3
+	pastEventsLookbackBlocks = 10000


Why don't we pass the block number from which the monitoring started to actFn? We can get the block number from the event in monitoringStartFn.

This is possible but I'm afraid this can lead to some problems in the future. I'm not sure if we can safely rely on blocks numbers taken from events due to some weird things which may happen like event duplications or chain reorgs. Also, this will work for all actions but provideRedemptionProof which should look in the past beyond the monitoring start block. Taking those concerns in account, I think the safest and simplest way is just start from block current - 10k

We can leave it like that for now. A little concerned about how much it costs to scan 10k previous blocks for operators using 3rd party Ethereum providers who charge by compute unit but we can revise in the future.

IIRC, it's always one eth_getLogs call if we are talking about the number of requests.

Yeah, though it sometimes does not matter if it's just one call but how expensive was to compute a respose for this call.

pkg/extensions/tbtc/tbtc.go

pkg/chain/local/local.go

pkg/chain/local/local_test.go

pkg/extensions/tbtc/tbtc.go

pkg/extensions/tbtc/tbtc_test.go

Stub of the provide redemption signature action

6d3e2da

Added a stub of the provide redemption signature monitoring process. Also, implemented an events log which will be needed at later stage.

lukasz-zimnoch requested a review from pdyraga October 22, 2020 12:01

lukasz-zimnoch changed the title ~~Stub of the provide redemption signature action~~ Redemption flow actions Oct 22, 2020

Provide redemption signature action

2fa23da

Implemented the main part of the redemption signature action.

pdyraga reviewed Oct 23, 2020

View reviewed changes

pkg/chain/ethereum/ethereum.go Outdated Show resolved Hide resolved

Base automatically changed from app-specific-actions to master October 23, 2020 08:48

Merge branch 'master' into redemption-actions

f84d2b0

# Conflicts: # pkg/extensions/tbtc/tbtc.go

pdyraga reviewed Oct 23, 2020

View reviewed changes

pkg/extensions/tbtc/events_log.go Outdated Show resolved Hide resolved

Simplify the iterator of past signature events

43292b4

pdyraga added the 📟 client label Oct 23, 2020

pdyraga added this to the v1.5.0 milestone Oct 23, 2020

pdyraga reviewed Oct 23, 2020

View reviewed changes

pkg/extensions/tbtc/tbtc.go Outdated Show resolved Hide resolved

pkg/extensions/tbtc/tbtc.go Outdated Show resolved Hide resolved

pkg/extensions/tbtc/tbtc.go Outdated Show resolved Hide resolved

pkg/extensions/tbtc/tbtc.go Outdated Show resolved Hide resolved

Remove the direct dependency on gen/abi

b13b76f

pdyraga reviewed Oct 23, 2020

View reviewed changes

lukasz-zimnoch added 4 commits October 23, 2020 12:21

Add line break on start event casting

c120196

Implementation of the redemption proof action

b5621ba

Implemented almost all elements of the provide redemption proof action. Also, some code refactoring has been done by the way.

Merge branch 'master' into redemption-actions

230da52

Set the right timeout for redemption proof action

8bf4841

pdyraga reviewed Oct 26, 2020

View reviewed changes

lukasz-zimnoch added 11 commits October 26, 2020 10:16

Move auxiliary types above monitorAndAct

db1536f

Change alias of the pkg/chain package

3396b77

Update tbtc and use the TBTCSystemEventLog

2307ccd

Rename BondedECDSAKeepFilterer

4482cbd

Add TODO about chain reorgs

513cc8f

Change signature lookup during action execution

01696f3

Optimize the start block when fetching past events

6c03e6b

Happy path test for provide redemption signature

1c9aa09

Add GoSec exception for local chain randomness

1d4bcc1

Add remaining tests for provide signature action

073fcf0

Happy path test for provide redemption proof

5c55f3c

lukasz-zimnoch added 2 commits October 27, 2020 13:48

Fix failing unit tests

46e1780

Add remaining tests for provide proof action

fa9e034

lukasz-zimnoch marked this pull request as ready for review October 27, 2020 14:01

pdyraga reviewed Oct 28, 2020

View reviewed changes

lukasz-zimnoch added 6 commits October 29, 2020 11:02

Tidy the go.sum file

a9a5a8d

Add comment to BlockTimestamp method

909ea90

Add TODO about rate limiter

d507bf5

Move TBTC chain interface to pkg/chain

10f673d

Fix import alias

2f77416

Add discussion link to the TODOs section

2f38d37

This was referenced Nov 2, 2020

Filter TBTC monitoring by operator interest #590

Merged

Improve Ethereum client wrappers adoption #594

Merged

pdyraga reviewed Nov 6, 2020

View reviewed changes

pdyraga mentioned this pull request Nov 6, 2020

Add TBTCSystem event log keep-network/tbtc#740

Merged

lukasz-zimnoch added 6 commits November 9, 2020 11:09

Remove signature submitted event lookup loop

c3df0ec

Align mutex names in local chain implementations

5d17df5

Add ctx param to local chain constructor

d869e2d

Add test for local SubmitSignature method

2dca954

Merge branch 'master' into redemption-actions

8cb25fd

Add comment about IncreaseRedemptionFee call

9f67bfa

pdyraga approved these changes Nov 9, 2020

View reviewed changes

pdyraga merged commit 5498656 into master Nov 9, 2020

pdyraga deleted the redemption-actions branch November 9, 2020 16:43

pdyraga mentioned this pull request Nov 10, 2020

Introduce custom Ethereum client interface keep-network/keep-common#55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redemption flow actions #585

Redemption flow actions #585

lukasz-zimnoch commented Oct 22, 2020 •

edited

Loading

pdyraga Oct 23, 2020

pdyraga Oct 26, 2020

lukasz-zimnoch Oct 26, 2020

pdyraga Oct 28, 2020

pdyraga Nov 6, 2020

pdyraga Oct 26, 2020

lukasz-zimnoch Oct 26, 2020

pdyraga Oct 28, 2020

lukasz-zimnoch Oct 29, 2020 •

edited

Loading

pdyraga Nov 6, 2020

lukasz-zimnoch commented Oct 27, 2020

pdyraga Oct 28, 2020

lukasz-zimnoch Oct 29, 2020

pdyraga Oct 29, 2020

lukasz-zimnoch Oct 29, 2020

pdyraga Nov 6, 2020

lukasz-zimnoch Nov 9, 2020

pdyraga Nov 9, 2020

pdyraga Nov 9, 2020

pdyraga Oct 28, 2020

pdyraga Oct 28, 2020

pdyraga Oct 28, 2020

lukasz-zimnoch Oct 29, 2020

pdyraga Nov 6, 2020

lukasz-zimnoch Nov 9, 2020

pdyraga Nov 9, 2020

Redemption flow actions #585

Redemption flow actions #585

Conversation

lukasz-zimnoch commented Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-zimnoch Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-zimnoch commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-zimnoch commented Oct 22, 2020 •

edited

Loading

lukasz-zimnoch Oct 29, 2020 •

edited

Loading