Cron ticker implementation #837

husio · 2019-06-28T10:11:49Z

Implement a message cron ticker.

This is an incomplete implementation. I want to show my approach first
to make sure it is correct before writing the implementation.

There are some decistions I had to make that I am not sure they were the
right ones.

When scheduling a task, we schedule a message for later execution.
When time will come, a phony transaction containing this message is
executed.
Because I am using a phony transaction, no transaction attributes can
be enforced.
All messages are executed in a single transaction. This means that a
single broken message can stop the whole queue from progressing.
All handler responses are ignored as the weave.Ticker interface
does not return almost anything on success.

This is work necessary for #671

iov-bot-danger · 2019-06-28T10:17:26Z

	1 Error
🚫	Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

Generated by 🚫 Danger

ethanfrey · 2019-06-28T13:35:34Z

All messages are executed in a single transaction. This means that a single broken message can stop the whole queue from progressing.

We should run each in a separate CacheWrap like we do with Deliver. If one job fails, it should be marked an error and removed from queue, and others can continue to run on future runs.

ethanfrey · 2019-06-28T13:38:54Z

All handler responses are ignored as the weave.Ticker interface does not return almost anything on success.

Let's update this. As of tendermint 0.14 or whenever I wrote this, BeginBlock returned nothing. It now returns tags: https://github.com/tendermint/tendermint/blob/v0.31.5/abci/types/types.proto#L167-L169

I would then allow DeliverResult response. We can use an error to signal discarding the cachewrap (similar to Savepoint Decorator). And from the positive result, just include the tags in the BeginBlock return value.

ethanfrey

Nice start and good queue design.

Some comments to help proceed

ethanfrey · 2019-06-28T13:40:30Z

queue/queue.go

+		if ok, err := db.Has(key); err != nil {
+			return errors.Wrap(err, "cannot check key existance")
+		} else if ok {
+			// If the key is already in use, instead of storing a


👍

Fair tradeoff. We need to ensure this queue cannot be spammed in any case

ethanfrey · 2019-06-28T13:41:41Z

queue/queue.go

+	// it := db.Iterate(since, until)
+	// ...
+
+	// TODO: iterate from 0 a.d. till now and return the first message


Sounds like a good approach. Clear TODO comment

ethanfrey · 2019-06-28T13:43:10Z

queue/queue.go

+
+func queueKey(t time.Time) []byte {
+	rawTime := make([]byte, 8)
+	binary.BigEndian.PutUint64(rawTime, uint64(t.UnixNano()))


Nanosecond? We could round off to second.

That limits how many messages can be processed per block to 1/second of block time. Which is actually a nice feature in my opinion.

Either is fine, just comment on the precision

I have added the comment and changed the granularity to second in 3c31063

You mentioned needing some reliable id to identify these issues. Does it need to be a primary key with an index based on time? Or can we just add some arbitrary "id" label to the payload (not just Msg)?

Let us think a bit more on this one. You also realize

var msg Msg msg.Unmarshal(data)

will always fail, as we need a concrete instance... which means we need some oneof casting here. (This is the reason go-amino allows runtime registration on interfaces. It does make coding in go easier, but at the cost of an undocumented and unmaintainable api for the client)

ethanfrey · 2019-06-28T13:46:54Z

queue/queue.go

+// Put queues the message in the database to be executed at given time.
+// Due to the implementation details, message is guaranteed to be executed
+// after given time, but not exactly at given time.
+func Put(db weave.KVStore, runAt time.Time, msg weave.Msg) error {


This is good.
I like serializing the msg and registering a handler (similar to gov).

Thinking this through, I realized something is missing and I have no clear way forward.... Authentication info (aka context). Is there a way to serialize context (using interface keys, I highly doubt it)?

We need to know what authority the transaction has when executed, and things like TextResolutionMsg must have other context, like the successful proposal that triggered it. You can complete this PR and merge without context, but it is essential to support serialized authorization somehow. I admit that is a large problem, so just getting a working queue merged in is a great step.

Should I serialize a transaction instead? This way there is no difference between executing a transaction straight away and executing with a delay.

The context is not the actual transaction. Transactions encapsulate requests coming from outside the blockchain. All items here originated inside the blockchain, from a trusted module. That means we don't need to evaluate signatures, but do need a way to capture all context.

I reflected some on #822 (comment) and while I had an adverse reaction to storing auth info like that, as it is no longer tied to any modules (insecure), if we do just run trusted modules and pin/audit their codebase, someone entering fake credentials should not be a real issue. And we can just store the contexts in a straight-forward method to remove much complexity.

This example would allow us to just serialize the AuthN []Condition or AuthN []Address items and reuse them on a new transaction. Maybe we should revisit this PR with an expanded request interface that makes it easy to do so?

queue/queue.go

cron/cron.go

husio · 2019-07-02T14:49:24Z

x/cron/cron.go

+
+			taskCtx := ctx
+			if prep, ok := task.(ContextPreparer); ok {
+				taskCtx = prep.Prepare(taskCtx)


@ethanfrey I am trying to figure out a good way to insert authentication (and other) information into the context. My latest approach is to delegate this to the client.

I have realized that this cannot be delegated to the client completely, because it is heavily dependent on the authentication used by the router. To keep things simple I was planning to use the same router as the rest of the application. This means, that the same sigs and multisig authentication is required.

I wonder if it would not be best to use the same technique for the Task implementation - create a message with middleware fields like multisig and signatures and use extension decorators to work with them. This make the whole functionality very much like a synchronous request handling, which is good.
The problem with this approach is that extension cannot set Task attributes because it does not know about the implementation. 🤔

I feel a bit like I am going in circles with this implementation. I did many improvements compared to the initial version, but now I slowed down and don't quite know how to solve the context issue.

Implement a message cron ticker. This is an incomplete implementation. I want to show my approach first to make sure it is correct before writing the implementation. There are some decistions I had to make that I am not sure they were the right ones. 1. When scheduling a task, we schedule a message for later execution. When time will come, a phony transaction containing this message is executed. 2. Because I am using a phony transaction, no transaction attributes can be enforced. 3. All messages are executed in a single transaction. This means that a single broken message can stop the whole queue from progressing. 4. All handler responses are ignored as the `weave.Ticker` interface does not return almost anything on success.

ruseinov · 2019-07-04T09:15:22Z

Briefly looked over this - the approach seems good to me.
I will return to this and try running tests locally and viewing code in the IDE to get a better understanding of the details.

husio · 2019-07-04T13:26:30Z

I am closing this draft as a I have created a new pull request using a cleaned up commits #850

husio requested a review from ethanfrey June 28, 2019 10:11

husio changed the title ~~This is a work in progress for #671~~ Cron ticker implementation Jun 28, 2019

ethanfrey reviewed Jun 28, 2019

View reviewed changes

husio force-pushed the msg_cron_issue_671 branch from 3c31063 to d942962 Compare July 1, 2019 08:01

husio commented Jul 1, 2019

View reviewed changes

cron/cron.go Outdated Show resolved Hide resolved

husio commented Jul 2, 2019

View reviewed changes

husio added 12 commits July 3, 2019 09:09

do not Check in cron

f2d3b76

use seconds as the time granularity

480773d

execute each mesage in its own db transaction

cc109db

use double cache for the cron task processing

e60cdd6

struggle with testing

73ed40f

more progress with cron implementation

f4efdaa

move cron package to x/

862ae91

include task result

21d9b3b

onward!

de86a8f

improve interfaces

dc4c85a

stuff

5583e37

husio force-pushed the msg_cron_issue_671 branch from cf25940 to 5583e37 Compare July 3, 2019 07:28

husio added 4 commits July 3, 2019 09:39

panic on failure

dee32ae

improve panic message

b52a72a

implement CronTask in bnsd

838d278

missing files

b23d6de

This comment has been minimized.

Sign in to view

husio added 3 commits July 3, 2019 12:55

yet another approach

2f93838

almost there

00cfa78

getting closer!

125ae5f

husio added 3 commits July 4, 2019 11:41

cleanup

3cfdcb3

I can smell the success!

53e77b8

acceptance tests passing

a4bc068

husio mentioned this pull request Jul 4, 2019

Implement cron functionality for messages. #850

Merged

husio closed this Jul 4, 2019

husio deleted the msg_cron_issue_671 branch July 24, 2019 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cron ticker implementation #837

Cron ticker implementation #837

husio commented Jun 28, 2019

iov-bot-danger commented Jun 28, 2019

ethanfrey commented Jun 28, 2019

ethanfrey commented Jun 28, 2019

ethanfrey left a comment

ethanfrey Jun 28, 2019

ethanfrey Jun 28, 2019

ethanfrey Jun 28, 2019

husio Jul 1, 2019

ethanfrey Jul 1, 2019

ethanfrey Jun 28, 2019

husio Jul 1, 2019

ethanfrey Jul 1, 2019

husio Jul 2, 2019 •

edited

Loading

This comment has been minimized.

ruseinov commented Jul 4, 2019

husio commented Jul 4, 2019

Cron ticker implementation #837

Cron ticker implementation #837

Conversation

husio commented Jun 28, 2019

iov-bot-danger commented Jun 28, 2019

ethanfrey commented Jun 28, 2019

ethanfrey commented Jun 28, 2019

ethanfrey left a 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

husio Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

ruseinov commented Jul 4, 2019

husio commented Jul 4, 2019

husio Jul 2, 2019 •

edited

Loading