ShouldSendMessage decider

[feuGeneA]

Why this should be merged

for #363

How this works

the relayer is informed of a decider service via config values that point to its host and port, and it communicates with that service process via grpc to make the decision.

How this was tested

a simple decider implementation is included and incorporated into the tests

[bernard-avalabs]

Generally makes sense to me. I left a few comments.

[bernard-avalabs]

Looks like this comment is duplicated on line 254. Was that intended?

[feuGeneA]

I moved it, but I think the original crept back in as a merge conflict. Thanks for pointing it out. Addressed in 5c6422e

[bernard-avalabs]

I believe WaitForStateChange is currently marked as experimental:

https://pkg.go.dev/google.golang.org/grpc#ClientConn.WaitForStateChange

Not sure if this is an issue, just wanted to point it out.

[feuGeneA]

thanks for pointing that out, I removed it in 584556e#diff-327181d0a8c5e6b164561d7910f4eeffd41442d55b2a2788fda2aa2692f17ec0

[bernard-avalabs]

Should this be initialized to false? It looks like the error cases are just returning decision without modifying it, and I assume we should return false in those cases.

[cam-schultz]

The intended control flow here is to separate the existing ShouldSendMessage decision logic from the Decider logic that is dispatched to the service. If the Decider fails, then we should return the prior decision from the existing logic.

I think this could be made clearer by adding a new helper, and calling it like so:

ShouldSendMessage() {
  // Existing decision logic
  decision, err :=  m.dispatchToDecider()
  if err != nil
    // log the error, but do not return it
    log.Warn("Decider returned error")
    return true, nil  
  }
  return decision
}

[feuGeneA]

I extracted the logic into a new helper, and adapted my changes to ShouldSendMessage to fit the existing flow logic (if __ return false; if ___ return false; ...; return true), in 584556e

[geoff-vball]

I don't think this is necessary. If ginkgo exits for whatever reason, everything that it was running should be cleaned up - we don't currently look for OS signals for the other processes we run.

What we do need is something that watches the decider process, and if it exists abnormally, we should panic. Example here https://github.com/ava-labs/awm-relayer/blob/main/tests/utils/utils.go#L91

[feuGeneA]

Are you sure it's not necessary? A little while ago I tried interrupting a test with Ctrl+C, and from the stdout it didn't look like it ran the AfterSuite hook, and the decider process was still running. And just now I tried again and saw that it did execute that hook, and the decider process was not running. So I think the behavior is inconsistent.

If the decider is left running/orphaned/defunct after the tests have finished, then it will fail to start in the next test run, because the port it uses is always the same and that port will be in use if it's already running from a previous test run. And if you're working on some changes totally unrelated to the decider, it could be pretty confusing and unclear what's going on.

This uncertainty and fear of confusion is why I decided to put this code into place. I'm happy to defer to the team's opinion though.

The idea of watching the decider for abnormal exit is a great idea, thanks, I'll do that.

[geoff-vball]

Yeah, I think we should move to running our e2e tests in a docker container (if feasible), that way we get the cleanup for free, and wouldn't need to check for OS signals, but until then, this is helpful.

[cam-schultz]

Is the github token necessary? Does the repo even have access to a gh token?

[feuGeneA]

Apparently not. I removed this in ed3f4e7 and everything still ran fine. I had just copied this from avalanchego. Thanks for noticing it.

[cam-schultz]

What's the difference between this method of checking that generated files are checked in versus the one we use in teleporter? https://github.com/ava-labs/teleporter/blob/main/.github/workflows/abi_bindings_checker.yml#L41

[feuGeneA]

not sure to be honest. I pulled this method from https://github.com/ava-labs/avalanchego/blob/49299868db0bdf5b3755c58993cc728173b3eedd/.github/workflows/check-clean-branch.sh#L8-L9

[cam-schultz]

Is buf required even if the Decider is unused (i.e. the relevant config options are omitted)?

[feuGeneA]

In order to build, yes, because the relayer binary is built to be aware of the protobuf interfaces so that that same binary can be used later if you decide you do want to include those config options.

[cam-schultz]

Let's add descriptions of the new config options.

[cam-schultz]

Can you add some validation on these new fields? If they're provided, they should be able to construct a valid URI.

[feuGeneA]

addressed in a9c374c

[cam-schultz]

Can we move this to main so that grpcClient can be limited in scope to main? When we get to #365 we'll have to handle dialing different grpcClients per message type, so some refactoring will be necessary anyway.

[feuGeneA]

addressed in b01b14b

[cam-schultz]

The intended control flow here is to separate the existing ShouldSendMessage decision logic from the Decider logic that is dispatched to the service. If the Decider fails, then we should return the prior decision from the existing logic.

I think this could be made clearer by adding a new helper, and calling it like so:

ShouldSendMessage() {
  // Existing decision logic
  decision, err :=  m.dispatchToDecider()
  if err != nil
    // log the error, but do not return it
    log.Warn("Decider returned error")
    return true, nil  
  }
  return decision
}

[cam-schultz]

Let's include timeouts in this PR

[feuGeneA]

addressed in f89bd35

[cam-schultz]

Is the intent behind v1 to map awm-relayer versions to Decider versions? If so, I'd prefer if we followed the pattern used by Avalanchego / subnet-evm where protocol version compatibility is provided out-of-band. Otherwise, the codebase would continue to grow with each new version.

[feuGeneA]

the intent was just to follow the normal/standard behavior/conventions of buf+proto, at least as our starting point here. I stripped all of the custom lint rules out of the buf config file when copying it from avalanchego, and one of those was about needing to include the version in the service name.

in 3631b7b I restored that linter rule and changed things to not include the version in the service/package names.

[michaelkaplan13]

Just calling this out because I wasn't aware how it worked exactly myself:

AvalancheGo communicates with VM plugins via gprc, but instead of assuming the VMs are running and providing their host/port in the configuration, it starts the configured VMs itself on the first port available on 127.0.0.1 here.

That type of pattern may be less error prone for relayers to set up. i.e: the relayer config would specify the decider plugin(s) (if any) to run for a given source chain, and the application would handle initializing/configuring them itself.

Not suggesting we do this now, but could be worth considering going forward.

[iansuvak]

I think that what Michael described is the generally preferred pattern for colocated services, and I agree that less config is better and less error prone.

A counter example for why we would want to keep it, is that this allows for using remote deciders.

[feuGeneA]

Cam and I also discussed this. For this iteration I was going for the simplest thing possible, and sub-process management proved non-trivial, so I went with this host/port approach instead, since we said that ultimately we want to be able to support both local and remote deciders.

Going forward, I'm open to doing the sub-process/plugin approach; we may be able to leverage the sub-process modules available in avalanchego, but if not then the complexity added to the code base for this would be a lot, at least as compared to the host/port approach.

Also, at the moment I'm leaning towards just the host/port approach so that there's one single config interface, rather than one for a local decider (the plugin path) and a separate/different one for a remote decider (the host/port).

[geoff-vball]

LGTM :)

[cam-schultz]

LGTM