-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Athena: Initial POC integration #6380
base: athena-poc
Are you sure you want to change the base?
Conversation
This functionality is being moved into the VM, or refactored here
and mock a few things while we wait for them to be implemented in Athena
We may need this later, to pass into the VM template
Host tests passing
Remove unneeded scaffolding
GetBalance test WIP
Upgrade to latest athena bindings Fix host call logic
Untested so far
Update athena go binding
fix encoding of verify args
Cleanup disabled tests and unneeded code
And fix a bug in the way genesis templates are bootstrapped
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## athena-poc #6380 +/- ##
============================================
- Coverage 80.3% 79.4% -0.9%
============================================
Files 328 330 +2
Lines 35765 42715 +6950
============================================
+ Hits 28752 33957 +5205
- Misses 5153 6812 +1659
- Partials 1860 1946 +86 ☔ View full report in Codecov by Sentry. |
@poszu this is ready for review. All tests are passing on ubuntu; mac tests are failing due to athenavm/athena#215 which is unrelated to the changes here. Sorry this is so big, but this is a minimum viable integration! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting an in-progress review - I will continue tommorow.
@@ -126,7 +126,7 @@ jobs: | |||
os: | |||
- ubuntu-22.04 | |||
- ubuntu-latest-arm-8-cores | |||
- macos-13 | |||
# - macos-13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to leave a note about why it's disabled and when it should be re-enabled:
# - macos-13 | |
# Athena doesn't support Intel Mac | |
# - macos-13 |
# - os: macos-13 | ||
# outname_sufix: "mac-amd64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
# - os: macos-13 | |
# outname_sufix: "mac-amd64" | |
# Athena doesn't support Intel Mac | |
# - os: macos-13 | |
# outname_sufix: "mac-amd64" |
@@ -1,10 +1,11 @@ | |||
package core | |||
|
|||
import ( | |||
"github.com/spacemeshos/go-scale" | |||
"github.com/ChainSafe/gossamer/pkg/scale" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the go-scale
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's harder to work with. It requires pre-generated scale types. With gossamer, you just pass in the data structure instance and get its serialization--as in Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already use go-scale
extensively and it's not been a problem so far. IMHO, consistency (using the same tool for encoding) is important. Probably the problem would be that go-scale by default encodes integers in the compact way. It supports non-compact encoding, but I'm not sure if it has a struct tag to switch to non-compact - perhaps this would need to be added. Or we could switch to compact encoding on the VM side.
if err != nil { | ||
return Address{}, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to panic in this situation, encoding should never fail. A lot of code would become simpler without this unnecessary error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless we later change encoding :)
func (t *Payload) EncodeScale(enc *scale.Encoder) (total int, err error) { | ||
{ | ||
n, err := scale.EncodeByteSlice(enc, *t) | ||
if err != nil { | ||
return total, err | ||
} | ||
total += n | ||
} | ||
|
||
return total, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (t *Payload) EncodeScale(enc *scale.Encoder) (total int, err error) { | |
{ | |
n, err := scale.EncodeByteSlice(enc, *t) | |
if err != nil { | |
return total, err | |
} | |
total += n | |
} | |
return total, nil | |
} | |
func (t *Payload) EncodeScale(enc *scale.Encoder) (int, error) { | |
return scale.EncodeByteSlice(enc, *t) | |
} |
logger, err := zap.NewDevelopment() | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider:
logger, err := zap.NewDevelopment() | |
require.NoError(t, err) | |
logger := zaptest.NewLogger(t) |
func TestEmptyCode(t *testing.T) { | ||
host, _ := getHost(t) | ||
defer host.Destroy() | ||
|
||
_, _, err := host.Execute( | ||
10, | ||
10, | ||
types.Address{1, 2, 3, 4}, | ||
types.Address{1, 2, 3, 4}, | ||
nil, | ||
0, | ||
[]byte{}, | ||
) | ||
|
||
require.ErrorContains(t, err, "athcon execute: no input code") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not testing any functionality implemented in go-spacemesh
but in the Athena bindings. I'd remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but that's testing the contract between node and athena lib. imo it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A contract that it is impossible to execute empty code? This test is already present in the athcon
library: https://github.com/athenavm/athena/blob/3f397a807111db7fc9a1ea9efd3be93bd3007730/ffi/athcon/bindings/go/athcon_test.go#L55-L66.
I mean, the check if code is empty is in the athcon
library (not anywhere in the go-spacemesh
) and it already has a test. It's a requirement imposed by the athcon
's vm.Execute()
method that the code cannot be empty. The user of the library doesn't need to verify if the library tests its inputs IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we say it tests whether errors are correctly surfaced from VM -> host? :)
func TestSetGetStorage(t *testing.T) { | ||
host, cache := getHost(t) | ||
defer host.Destroy() | ||
|
||
storageKey := athcon.Bytes32{0xc0, 0xff, 0xee} | ||
storageValue := athcon.Bytes32{0xde, 0xad, 0xbe, 0xef} | ||
|
||
address := types.Address{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1} | ||
account := types.Account{ | ||
Address: address, | ||
Balance: 10000, | ||
Storage: []types.StorageItem{ | ||
{Key: storageKey, Value: storageValue}, | ||
}, | ||
} | ||
err := cache.Update(account) | ||
require.NoError(t, err) | ||
|
||
_, gasLeft, err := host.Execute( | ||
account.Layer, | ||
100000, | ||
account.Address, | ||
account.Address, | ||
nil, | ||
0, | ||
hostprogram.PROGRAM, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, the host functionalities could be tested without running actual programs in the VM. The host has an API exposed to VM which abstracts things nicely and the host features could be tested by exercising this API.
In this test, it's not even possible to tell what the hostprogram.PROGRAM
will do because there is no source code for it (same for the get balance test above).
req := smocks.NewMockValidationRequest(ctrl) | ||
req.EXPECT().Parse().Times(1).Return(tx.TxHeader, nil) | ||
req := smocks.NewMockValidationRequestNew(ctrl) | ||
req.EXPECT().Cache().Times(1).Return(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Times(1)
and Return(nil)
are defaults, so it's equivalent to:
req.EXPECT().Cache().Times(1).Return(nil) | |
req.EXPECT().Cache() |
IMHO less is more and it's more readable (less cluttered) this way.
@@ -79,7 +82,7 @@ type VM struct { | |||
} | |||
|
|||
// Validation initializes validation request. | |||
func (v *VM) Validation(raw types.RawTx) system.ValidationRequest { | |||
func (v *VM) Validation(raw types.RawTx) system.ValidationRequestNew { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it return an interface instead of the Request
struct that implements it?
func (v *VM) Validation(raw types.RawTx) system.ValidationRequestNew { | |
func (v *VM) Validation(raw types.RawTx) *Request { |
It's against the common rule "accept interfaces, return concrete implementations". Is there a need for this?
tx2, err := wallet.Spend(signer.PrivateKey(), recipient, 1, nonce, sdk.WithGasPrice(0)) | ||
if err != nil { | ||
panic(err) | ||
} | ||
tx.RawTx = types.NewRawTx(tx2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As NewTx
is a test-only function, it could take tb testing.TB
and then this could be simplified to:
tx2, err := wallet.Spend(signer.PrivateKey(), recipient, 1, nonce, sdk.WithGasPrice(0)) | |
if err != nil { | |
panic(err) | |
} | |
tx.RawTx = types.NewRawTx(tx2) | |
tx2, err := wallet.Spend(signer.PrivateKey(), recipient, 1, nonce, sdk.WithGasPrice(0)) | |
require.NoError(tb, err) | |
tx.RawTx = types.NewRawTx(tx2) |
t.Method = uint32(tx.Method) | ||
// t.Method = uint32(tx.Method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is tricky in Athena. method is part of the opaque payload, and I want to hide that from the node and from the host as much as possible. we break this abstraction in a few places already, gently. we need to debate whether we want the API to parse this data for the client, or whether we should instead have it return the opaque payload and require the client to parse this using an athena SDK.
if spawnSelector, err := athcon.FromString("athexp_spawn"); err != nil { | ||
return res, txType, fmt.Errorf("%w: failed to create spawn selector: %w", core.ErrInternal, err) | ||
} else if spendSelector, err := athcon.FromString("athexp_spend"); err != nil { | ||
return res, txType, fmt.Errorf("%w: failed to create spend selector: %w", core.ErrInternal, err) | ||
} else if *method == spawnSelector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for else
branches, as the previous branch returns on error. Consider:
if spawnSelector, err := athcon.FromString("athexp_spawn"); err != nil { | |
return res, txType, fmt.Errorf("%w: failed to create spawn selector: %w", core.ErrInternal, err) | |
} else if spendSelector, err := athcon.FromString("athexp_spend"); err != nil { | |
return res, txType, fmt.Errorf("%w: failed to create spend selector: %w", core.ErrInternal, err) | |
} else if *method == spawnSelector { | |
spawnSelector, err := athcon.FromString("athexp_spawn") | |
if err != nil { | |
return res, txType, fmt.Errorf("%w: failed to create spawn selector: %w", core.ErrInternal, err) | |
} | |
spendSelector, err := athcon.FromString("athexp_spend") | |
if err != nil { | |
return res, txType, fmt.Errorf("%w: failed to create spend selector: %w", core.ErrInternal, err) | |
} | |
switch *method { | |
case == spawnSelector: | |
// body | |
case == spendSelector: | |
// body |
// TODO(lane): Method is unused by the Athena VM, and should be removed. | ||
Method uint8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep it?
type ValidationRequestNew interface { | ||
Parse(core.AccountLoader) (*types.TxHeader, error) | ||
Verify() bool | ||
Cache() *core.StagedCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the need for Cache()
. It's only used as a parameter for Parse()
as far as I can tell. Why have it?
// sig := ed25519.Sign(ed25519.PrivateKey(pk), core.SigningBody(options.GenesisID[:], tx)) | ||
sig := ed25519.Sign(ed25519.PrivateKey(pk), tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant that there is // sig := ed25519.Sign(ed25519.PrivateKey(pk), core.SigningBody(options.GenesisID[:], tx))
left. I understand that genesis should be included - I'm asking what the plan is for it. If not, the commented line could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to decide how we want to implement genesisID support in Athena. Assuming we do it in a way that's compatible with what go-spacemesh does today, then these commented lines can just be restored once that's been added on the Athena side.
// tx := encode(&sdk.TxVersion, &principal, &meta) | ||
// tx = append(tx, payload...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// tx := encode(&sdk.TxVersion, &principal, &meta) | |
// tx = append(tx, payload...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave the commented-out code here when it's clear that we need to add it back later (in this case, once we have athenavm/athena#178)
// sig := ed25519.Sign(ed25519.PrivateKey(pk), core.SigningBody(options.GenesisID[:], tx)) | ||
sig := ed25519.Sign(ed25519.PrivateKey(pk), tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
// tx := encode(&sdk.TxVersion, &principal, &meta) | ||
// tx = append(tx, payload...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// tx := encode(&sdk.TxVersion, &principal, &meta) | |
// tx = append(tx, payload...) |
func ComputePrincipalFromPubkey(template types.Address, pubkey signing.PublicKey) (Address, error) { | ||
// construct and encode the blob, which is a SCALE-encoded Athena wallet template instance | ||
blob, err := scale.Marshal(struct { | ||
Nonce, Balance uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need Balance
here?
IMO it looks weird to add balance as a "hidden spawn argument" for the Wallet account type, it's not a vault or some token smart-contract (total issuance)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, also I guess such Nonce
field should be a part of payload of spawn tx, and it's the different nonce from the "tx counter"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, as discussed in Slack, nonce and balance should be removed -- not just from here, but from the VM side as well.
this struct just mimics the Athena wallet program's struct. rather than encoding it manually here, we might choose instead to add an exported method to Athena that performs this serialization so we don't need to keep these two data structures/codecs in sync.
Also I suggest to use another TemplateAddress for Athena's Wallet to avoid collisions with the "genvm" template addresses, just to keep things simpler. |
// ComputePrincipal address as the last 24 bytes of Hash(template || blob). | ||
// See https://github.com/spacemeshos/go-spacemesh/issues/6420 for more details. | ||
func ComputePrincipalFromBlob(template types.Address, blob []byte) Address { | ||
hasher := hash.GetHasher() | ||
defer hash.PutHasher(hasher) | ||
encoder := scale.NewEncoder(hasher) | ||
template.EncodeScale(encoder) | ||
args.EncodeScale(encoder) | ||
hasher.Write(template[:]) | ||
hasher.Write(blob) | ||
sum := hasher.Sum(nil) | ||
rst := types.GenerateAddress(sum[12:]) | ||
return rst | ||
return types.GenerateAddress(sum) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if using types.GenerateAddress
is OK here, see its implementation:
go-spacemesh/common/types/address.go
Lines 132 to 139 in 40d4ba6
func GenerateAddress(publicKey []byte) Address { | |
var addr Address | |
if len(publicKey) > len(addr)-AddressReservedSpace { | |
publicKey = publicKey[len(publicKey)-AddressLength+AddressReservedSpace:] | |
} | |
copy(addr[AddressReservedSpace:], publicKey[:]) | |
return addr | |
} |
It does more than taking the upper 24B - it also reserves the lower 4B of the returned address (they are zeros)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it 20B (used) + 4B (reserved) = 24B total?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then that should be fine, since Athena also uses 24 byte addresses. I don't understand your initial comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it built from the example wallet in the athena repo? Why not have the sources of the wallet in go-sm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. definitely, can do, it's just duplicative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the one in athena repo is an example :) I think the right place for the wallet template code is go-spacemesh, even if it's a duplicate, to avoid breaking the devnet by some changes to the example wallet code in the athena repo.
vmhost, err := vmhost.NewHost(host) | ||
if err != nil { | ||
return 0, fmt.Errorf("loading Athena VM: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in few other places the VM is not destroyed (missing call to Destroy
)
vmhost, err := vmhost.NewHost(host) | |
if err != nil { | |
return 0, fmt.Errorf("loading Athena VM: %w", err) | |
} | |
vmhost, err := vmhost.NewHost(host) | |
if err != nil { | |
return 0, fmt.Errorf("loading Athena VM: %w", err) | |
} | |
defer vmhost.Destroy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
we should keep using the existing hardcoded scheme that we’re using in genVM. there is no conflict here. athena and genvm will never exist in the same namespace, even if athena runs as a L2 on top of spacemesh. for user-deployed templates, yes, we calculate the template address as a hash of the code, but this isn’t the case for harcoded templates, of which there must always be at least one (the single-sig wallet). we can debate whether we want to add other hardcoded templates later (e.g., a multisig), but it’s not strictly necessary as long as there’s one to “bootstrap” everything. |
Who would pay for deploying the other "basic" templates? I think it'd be easier to integrate if they were hardcoded in the genesis config. |
We would just deploy them ourselves at or shortly after genesis. We'd link to these deployed templates in the documents, in our SDKs, etc. - that's my understanding of how things work in, e.g., Solana, which has a standard template library. If we hardcode them, how do we handle upgrading the template later? |
Motivation
Replace existing genvm functionality with athena
Description
vm
codevm
template-related code with calls into athenaTest Plan
Includes almost all existing production tests. Disabled a few tests that aren't compatible with the Athena VM, and also added a few Athena-specific tests.
TODO
Misc
genvm
code remains but it's totally unused outside ofgenvm
tests.host
andcontext
can probably be unified in a future, Athena-native implementation. The goal here is expediency, not pretty, maintainable code.State
andStorage
blobs to the account.Storage
is implemented very naively, as an array rather than as a hash table, for simplicity of serialization.ath
andatest
for Athena transactions, and it changes the tx version byte to1
.