Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@marcio-diaz
Copy link
Contributor

@marcio-diaz marcio-diaz commented Jul 8, 2019

Initial proposal for an API for constructing transactions from Calls.
This will be used, for instance, for constructing misbehavior report transactions.

Context: #2181

Example usage

let signing_payload = runtime_api().signing_payload(block_id, report_call, public);
let signature = Pair::from_public(public).sign(signing_payload);
let tx = runtime_api().build_transaction(block_id, signing_payload, signature);
transaction_pool.submit_one(&block_id, tx);

Example definitions

Signing Payload

fn signing_payload(encoded_call: &[u8], account_id: &[u8]) -> Vec<u8> {
    let nonce = get_account_nonce(public);
    let call = Decode(encoded_call);
    let genesis_hash = ...; // From where?
    (nonce, call, Era::immortal(), genesis_hash).encode()
}

Build Transaction

fn build_transaction(signing_payload: &[u8], signature: AnySignature) -> Vec<u8> {
    let (nonce, call, era, ...) = Decode(signing_payload);
    let public = ...; // From where?
    UncheckedExtrinsic::new_signed(nonce, call, public, signature, era).encode()
}

@marcio-diaz marcio-diaz added the A0-please_review Pull request needs code review. label Jul 8, 2019
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some problems with the interface

@rphmeier rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels Jul 15, 2019
@kianenigma
Copy link
Contributor

fyi: the signature payload is changing in #3102.

bkchr
bkchr previously requested changes Jul 19, 2019
@bkchr
Copy link
Member

bkchr commented Jul 27, 2019

Some general questions, why do we need two methods? Don't you normally just want create a signed transaction in one call?

Next question would be, do we really need this? If we take the current design, we probably need 3 calls into the runtime for creating the transaction. Can we not directly return a transaction from the misbehavior construction call? As you always need to call into the runtime to get the correct call, we could just return the signed transaction and be done.

@rphmeier rphmeier mentioned this pull request Jul 28, 2019
10 tasks
@marcio-diaz
Copy link
Contributor Author

Some general questions, why do we need two methods? Don't you normally just want create a signed transaction in one call?

Next question would be, do we really need this? If we take the current design, we probably need 3 calls into the runtime for creating the transaction. Can we not directly return a transaction from the misbehavior construction call? As you always need to call into the runtime to get the correct call, we could just return the signed transaction and be done.

I think the problem is that we can't sign a transaction in no-std right now (although it should be possible, right? but can we assume it?). Then you need to make it as general as possible as @rphmeier commented above.

@marcio-diaz marcio-diaz requested review from bkchr and rphmeier July 29, 2019 09:24
@marcio-diaz marcio-diaz added A0-please_review Pull request needs code review. and removed A5-grumble labels Jul 29, 2019
@marcio-diaz marcio-diaz dismissed stale reviews from bkchr and rphmeier July 29, 2019 09:26

fixed

@Demi-Marie Demi-Marie added A5-grumble and removed A0-please_review Pull request needs code review. labels Jul 29, 2019
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding for this API is that the consensus engine needs a way to send transactions for reporting misbehavior. These transactions must be signed outside of the runtime (potentially with different keys and crypto from what the consensus engine is using).

How do we fill encoded_call parameter? Isn't that what we want to abstract over? Otherwise the consensus engine still needs to find a way to figure out which method to call (which is specific to the runtime). If you note on #2181, @rphmeier mentioned that consensus runtime APIs should get a method like construct_report_call which would return the encoded call.

@marcio-diaz
Copy link
Contributor Author

marcio-diaz commented Jul 30, 2019

How do we fill encoded_call parameter? Isn't that what we want to abstract over? Otherwise the consensus engine still needs to find a way to figure out which method to call (which is specific to the runtime). If you note on #2181, @rphmeier mentioned that consensus runtime APIs should get a method like construct_report_call which would return the encoded call.

Yes, encoded_call is the output of the construct_report_call for each consensus API. Is it OK?

I'm not sure that is enough though, I can imagine we may want to pass more parameters to the build_transaction function. I'll add an implementation of the functions to get a better idea.

@rphmeier
Copy link
Contributor

rphmeier commented Jul 30, 2019

Don't you normally just want create a signed transaction in one call?

Yeah, that would be nice. But the runtime controls transaction & signing format, not consensus code. I don't see an obvious way to get the runtime to sign transactions either.

@bkchr
Copy link
Member

bkchr commented Jul 30, 2019

Don't you normally just want create a signed transaction in one call?

Yeah, that would be nice. But the runtime controls transaction & signing format, not consensus code. I don't see an obvious way to get the runtime to sign transactions either.

By "one call", I mean on runtime call. I know that we need to call into the runtime anway, to at least get the correct Call. If we are already in the runtime, we can also create the transaction and sign it. Everything in one call. This would not even require an extra api for transaction building, as each consensus just can adds the function to its own runtime api interface.

@marcio-diaz marcio-diaz added A0-please_review Pull request needs code review. and removed A5-grumble labels Jul 31, 2019
@rphmeier
Copy link
Contributor

rphmeier commented Jul 31, 2019

If we are already in the runtime, we can also create the transaction and sign it. Everything in one call.

Why would consensus keys be available within the runtime? That would require some offchain magic.

Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, especially the runtime API. My one suggestion would be to consolidate the two functions into one function that takes a signing callback. I do not think this would make the consensus engine implementation significantly harder, and I do think it would significantly ease the jobs of runtime authors.

@bkchr
Copy link
Member

bkchr commented Aug 2, 2019

Why would consensus keys be available within the runtime? That would require some offchain magic.

With this pr: #3213 We will be able to sign from the runtime.

@marcio-diaz
Copy link
Contributor Author

Why would consensus keys be available within the runtime? That would require some offchain magic.

With this pr: #3213 We will be able to sign from the runtime.

Ok, then this PR is not needed and we can close, right?
I'll prepare the PR for adding the construct_calls to the consensus APIs.

@gavofyork
Copy link
Member

Closing for now. Can reopen if turns out to be needed.

@gavofyork gavofyork closed this Aug 8, 2019
@bkchr bkchr deleted the marcio/transaction-builder-api branch August 8, 2019 18:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants