Refactor: Unify EthTx to FilecoinMessage methods v2 #10095

geoff-vball · 2023-01-21T05:54:52Z

Related Issues

Rebases #10017 and #10026 onto release/v1.20.0

Proposed Changes

Mostly a refactor

When converting a non-eth transaction into an EthTx, it will now convert the to/from into eth addresses if possible.

You can still look up a BLS message by either the cid of the unsigned or signed (if you have it) message. We just now always use the unsigned cid for the conversion to a transaction hash. Then looking up by transaction hash, it will return the cid of the unsigned BLS message.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

Commits have a clear commit message.
PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
- area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

arajasek · 2023-01-23T16:01:00Z

node/impl/full/eth.go

 		tx.Hash, err = tx.TxHash()
 		if err != nil {
 			return tx, err
 		}
-	} else if smsg.Signature.Type == crypto.SigTypeUnknown { // BLS Filecoin message
+	} else if smsg.Signature.Type == crypto.SigTypeUnknown { // Executed BLS Filecoin message
+		tx = ethtypes.EthTxFromNativeMessage(smsg.VMMessage())


Hmm, this means there isn't a canonical EthTx representation of BLS messages -- depending on status, the hash may or may not include the BLS signature.

Is this the status quo today (irrespective of this PR)?

Is this desirable? Would it be better to say BLS messages never include the siggy in their EthTx hash?

That was the status quo, but I do think it is bad. I've changed the flow to always use the unsigned message for BLS messages to calculate it's EthTx hash.

I think that's an improvement, but we should get product insight here

@aashidham @raulk thoughts?

FYI, you can still look up a BLS message by either the cid of the unsigned or signed (if you have it) message. We just now always use the unsigned cid for the conversion to a transaction hash. Then looking up by transaction hash, it will return the cid of the unsigned BLS message.

node/impl/full/eth.go

raulk · 2023-01-26T20:57:32Z

chain/types/ethtypes/eth_transactions.go

+// - TransactionIndex
+// - From
+// - Hash
+func EthTxFromSignedEthMessage(smsg *types.SignedMessage) (EthTx, error) {


Eth{...}Message is a confusing name as it leads to thinking that this is an Eth message/tx, but it's actually a Filecoin one.

Suggested change

func EthTxFromSignedEthMessage(smsg *types.SignedMessage) (EthTx, error) {

func EthTxFromSignedMessage(smsg *types.SignedMessage) (EthTx, error) {

We kind of need some way of differentiating between types.Message/types.SignedMessage that can be converted into a legitimate EthTx or not. It makes sense to me to use Transaction/Tx for the Ethereum shape, and Message for the Filecoin shape, but I know other people might have differing thoughts about those words.

I just want to avoid the confusion of someone thinking they can use this function for any signed message.

raulk · 2023-01-26T20:57:45Z

chain/types/ethtypes/eth_transactions.go

+	}, nil
+}
+
+func EthTxArgsFromUnsignedEthMessage(msg *types.Message) (EthTxArgs, error) {


Suggested change

func EthTxArgsFromUnsignedEthMessage(msg *types.Message) (EthTxArgs, error) {

func EthTxArgsFromUnsignedMessage(msg *types.Message) (EthTxArgs, error) {

node/impl/full/eth.go

raulk · 2023-01-26T21:15:48Z

node/impl/full/eth.go

+		tx.From, err = lookupEthAddress(ctx, smsg.Message.From, sa)
+		if err != nil {
+			return ethtypes.EthTx{}, xerrors.Errorf("failed to lookup from ethaddress: %w", err)
+		}


Where does the To address get converted now?

The To address get populated in EthTxArgsFromUnsignedEthMessage. Any messages sent this way have to be for f0 or f4, which means we don't need to looks anything up in state, we just convert them using EthAddressFromFilecoinAddress.

I might be missing an edge case here that I haven't thought of though. What are your thoughts?

Come to think of it, we should be able to do the same for From in this case, as it should always be an f4? It's only for Messages sent in native form that we might have to look up, say, the ID address of an f1 and then transform it into our eth-equivalent.

raulk

This looks good. Gets rid of some cruft related to old EAM method that were invokable externally and are no longer so, superseded by CreateExternal. I think there's a lot more consolidation/cleanup that we can still do.

For example, I would like to see the EthTxArgs type be renamed to EthRlpTx and functions associated with that type as members of the new type.

And there's more cleanup possible within the various conversion functions.

raulk · 2023-01-30T18:40:23Z

The failing test is a flaky test.

…in-project#71) * Test and fix eth_FeeHistory * Add test for eth_getStorageAt * Add test for eth_call * Change EthBlock.Extradata type to EthBytes * fix: EthAPI: Drop hack in GetStorageAt * Eth JSON-RPC: implement web3_clientVersion * fix lint * add genesis for ipc gateway * Remove debug logging * chore: cli Standarize cli/code functions similar to: filecoin-project#9317 - cctx.NArg() instead of cctx.Args().xxx - Add check for args and print help on functions that did not have it * fix: rpcenc: Don't hang when source dies * make docsgen-cli make docsgen-cli * fix: EthAPI: Handle EthCall return correctly * feat: vm: Assert empty object CID when dumping state * Update codeql to v2 * Enable code ql for release branches * less strict ArgsCheck Less strict ArgsCheck * feat: wdpost: Emit more detailed errors * Fix panic in EthGetCode * Update codeql to v2 * Enable code ql for release branches * Add actors to circ-supply - Consider funds to EAM as locked - Add evm, placeholder and ethaccount to circ supply * devnets starts with nv17 * Use new kit.DisableEthRPC method in test * make gen * init git submodules in build-docker CI * Remove old Snapcraft and AppDir folders * Always include checked in files in docker context The .dockerignore file is symlinked to the .gitignore file, and checked in files should not be removed from our docker context otherwise they result in dirty git state when we build our images. * Check git state in Dockerfile to catch .dockerignore bugs * make jen - Add builtin.EthereumAddressManagerActorAddr to builtin.go.template and make jen - Rename to EthereumAddressManagerActorAddr to match pattern of other actors (CronActorAddr/etc) * chore: all: bump go-libipfs to replace go-block-format Includes changes from: - ipfs/go-block-format#37 - ipfs/boxo#58 * init git submodules in build-docker CI * Remove old Snapcraft and AppDir folders * Always include checked in files in docker context The .dockerignore file is symlinked to the .gitignore file, and checked in files should not be removed from our docker context otherwise they result in dirty git state when we build our images. * Check git state in Dockerfile to catch .dockerignore bugs * Fix lint errors * Skip some rpc conformance tests * make gen * Changed skip message * fix: don't replace the method in EthSendRawTransaction (filecoin-project#10129) This will just cause signature validation to fail. * Remove stm: #integration comment * Fix comment * fix: devnets: default to starting from nv17 * Refactor to ensure conformance test can run in circleci * go fmt * Standardize path variable * Update to go-state-types v0.10.0-alpha-11 * chore: node: migrate go-bitswap to go-libipfs/bitswap This was migrated in ipfs/boxo#63. * simplify transaction hash db queries, prevent fd leaks * Refactor: Unify EthTx to FilecoinMessage methods v2 (filecoin-project#10095) * Refactor: Unify EthTx to FilecoinMessage methods * Filecoin messages can again be converted to Eth Txs * All BLS messages should calculated tx hash with unsigned message * Refactor newEthTxReceipt * fill in from and to for non-eth transactions * Hoist nil check out of newEthTxFromMessageLookup --------- Co-authored-by: Aayush <[email protected]> Co-authored-by: Raúl Kripalani <[email protected]> * Populate transaction hash database if the database has not been set up before * rpc: Switch eth_subscribe to reverse calls * gateway: eth_subscribe support * itests: Fix TestEthSubscribeLogs * ethtypes: Serialize EthFilterID/EthSubscriptionID correctly * eth rpc: Params are optional in eth_subscribe * fix: cli: add ArgsUsage fix: cli: add ArgsUsage * make libp2p user agent overridable * move UserAgentOption upwards * Eth JSON-RPC: from in eth_getTransactionByHash is not correctly populated filecoin-project#1614 * Move assigning `from` address to the API level * Improve the error message * Add test for EthGetTransactionByHash * fix: should not serve non v0 api in v1 * Fix: typo Fix: typo * Fix: typos Fix: typos * Fix: typo Fix: typo * Fix: typo Fix: typo * Fix: typo Fix: typo * Fix: typos Fix: typos * Fix: typos Fix: typos * test: fevm: add in tests for deploying, destroying contracts, recursive calls, sending value (filecoin-project#10082) adds the following tests to itests/fevm_test.go: - recursive tests - delegate call tests - delegate call recursive tests - revert tests - destruct tests - contract deploy address tests - send value to contracts - gas limit on value transfer tests - sending value to destroyed contracts adds the test to itests/fevm_address_test.go: - deploy contract and confirm address is different second deploy * merge * tests that use create2 and destroy to validate evm state * wip: ipc_gateway in genesis * deps: Update go-jsonrpc to v0.2.1 * Test: assert all fields returned from EthGetTransactionByHash have expected values Related to filecoin-project#10151 (comment) * Fix merge conflicts * Allow f4 address to send to all address types if ID address exists on chain * fix: extend LOTUS_CHAIN_BADGERSTORE_DISABLE_FSYNC to the markset Without doing this walking a badger markset on a non-nvme knocks the node hopelessly out of sync during a compaction. * review fixes * gateway: Support all EthModule methods * node builder: Use gateway eth module in lite mode * feat: ethrpc: Support filtering by address in subscribe * itests: Fix TestEthFilterAPIDisabledViaConfig * Check decoding params for new methods * Review fixes * feat: evm: align events implementation with FIP-0049 and FIP-0054. (filecoin-project#10152) - Event keys are now t1, t2, t3, t4 for topics; and d for data. - ref-fvm no longer stores events in the blockstore for us. It just returns events to the client, who is now responsible for handling them as it wishes / according to its configuration. - Add a flag to VMOpts to have the events AMT be written in the blockstore. - Add a flag to the ChainStore to advertise to the rest of the system if the ChainStore is storing events. - Enable that flag if the EthRPC is enabled (can also add an explicit configuration flag if wanted). * cli: fix extend cmd to get the right sector number * todo: rebase master to include release/v1.20.0 * feat: compute a better gas limit for recursive external contract calls * make gen * fix: ethtypes: Correct 'no uncles' hash in NewEthBlock * LOTUS_FEVM_ENABLEETHRPC: Fix env variable name in error * fix: ethtypes: Correct 'no transactions' hash in NewEthBlock * fix: null rounds: pass correct timestamp to the FVM. * itest: fix FEVM tests for upstream changes * itest: fix: test comment * itest: fix remaining fevm failures * improve evm error handling in itests (filecoin-project#10161) * use WithValue language for test * clean up test for recursive delegate call count. improved readability (filecoin-project#10195) * chain: explicitly check that gasLimit is above zero * add bundle git tag from pack.sh into builtin_actors_gen * fix: worker: add all task type flag Add all flag for the `lotus-worker tasks enable/disable` cmd * Update cmd/lotus-worker/tasks.go Co-authored-by: Łukasz Magiera <[email protected]> * Update cmd/lotus-worker/tasks.go Co-authored-by: Łukasz Magiera <[email protected]> * Update cmd/lotus-worker/tasks.go Co-authored-by: Łukasz Magiera <[email protected]> * make docsgen-cli make docsgen-cli * retry make docsgen-cli retry make docsgen-cli with the new usage * remove bundle-gen from make gen - not actually useful today anyway * fvm: chore: update FVM This: 1. Updates the builtin actors bundle (for actors v10). 2. Updates the event entry type to include the codec. 3. Removes the cbor encoding and zero trimming from event data. I've chose to: 1. _Not_ add codec handling to the event filtering system for now. 2. _Skip_ events with unexpected codecs. We don't actually _allow_ these events in the FVM right now, and it simplifies the implementation. However, I _am_ recording the codecs in the database so we don't have to migrate it later. * Option to set-seal-delay seconds Add the option to set-seal-delay in seconds * make docsgen-cli make docsgen-cli * Error if backup file exists Error out if backup file already exists * fix: ethrpc: Don't send sub notifs in array * Update eth_subscribe itests to work with correct responses * test: build: ignore git tags in embedded metadata * fix: build: use actual v9 bundles for butterfly * fix: ethrpc: emit correct bloom filters (filecoin-project#10194) * skip problematic test vectors. * feat: actors: Add bytecode getter * itests: Test EVM bytecode getters, check bytecode hash * ethrpc: Add missing fields to EthTx * Update cli/backup.go Co-authored-by: Łukasz Magiera <[email protected]> * feat: eth cli: Strip out empty spaces around contract bytes * fix: chain: make sure the head is empty, not the code * bump test vectors. * remove test vectors bleeding edge job. We no longer need it because specs-actors is deprecated. v7 vectors have been merged to master. * fix: Don't call WalletExport in msg signing flows * updates butterflynet reset artifacts * bootstrap node multiaddr * new genesis file these files are necessary for others to build lotus and join the new butterflynet, reset on February 08, 2023 * fix: gas: update ffi & correct the message inclusion cost in nv18 (filecoin-project#10228) Co-authored-by: Raúl Kripalani <[email protected]> * refactor: use EthHash for event topics This ensures they're always 32 bytes and padded, as required. * fix: eth: strict event parsing We now enforce the following rules: 1. No duplicate topics or data. 2. Topics must have 32 byte keys. 3. Topics may not be skipped. (e.g., no t1 & t3 without a t2). 4. Raw codecs. We _don't_ require that topics/data be emitted in any specific order. We _skip_ events with unknown keys. We _drop_ events that violate the above rules. * Apply suggestions from code review Co-authored-by: raulk <[email protected]> * fix: eth: log on unexpected events We can remove these later as we add more event types, but this will aid in debugging. * fix: make gen * fix config.yml * implement itest and handle optional params * fix typo * fix: stmgr: make the tipset and height agree when estimating gas (filecoin-project#10216) * fix: stmgr: make the tipset and height agree when estimating gas Specifically re-execute all messages in the current tipset, tacking the new message onto the end. That way, the epoch is the epoch of the current tipset. We could try to "make" a fake block and use that, but that's unlikely to work well. * fix: stmgr: only apply tipset messages for CallWithGas * fix: itest: window post dispute * eth_feeHistory: parse block param correctly. * add todo * Update to FFI v1.20.0-rc1 * fix bad test. * improve TODO. * Update to go-state-types v0.10.0-rc2 * disable adding git tags to bundle metadata * Update actors to v10.0.0-rc.1 * eth: FIP-0055: implement final version of transitory delegated signature. (filecoin-project#10239) * make lint happy and re-generate devgen.car * setup ipc gateway in genesis and assign initial balance * override with ipc-actors type to load ipc bundle automatically * set balance of reward actor according to network name and minor fix * linter fixes * make linter happy :) * deploy ipc subnet actor * fix config.yml --------- Co-authored-by: Ian Davis <[email protected]> Co-authored-by: Aayush <[email protected]> Co-authored-by: ychiao <[email protected]> Co-authored-by: cryptoAtwill <[email protected]> Co-authored-by: Phi <[email protected]> Co-authored-by: Łukasz Magiera <[email protected]> Co-authored-by: Geoff Stuart <[email protected]> Co-authored-by: Jiaying Wang <[email protected]> Co-authored-by: Geoff Stuart <[email protected]> Co-authored-by: Łukasz Magiera <[email protected]> Co-authored-by: Jennifer Wang <[email protected]> Co-authored-by: Ian Davis <[email protected]> Co-authored-by: Jorropo <[email protected]> Co-authored-by: Steven Allen <[email protected]> Co-authored-by: Raúl Kripalani <[email protected]> Co-authored-by: Richard Guan <[email protected]> Co-authored-by: Maciej Witowski <[email protected]> Co-authored-by: omahs <[email protected]> Co-authored-by: snissn <[email protected]> Co-authored-by: Peter Rabbitson <[email protected]> Co-authored-by: 0x5459 <[email protected]> Co-authored-by: Travis Person <[email protected]> Co-authored-by: Anton Evangelatov <[email protected]> Co-authored-by: ognots <[email protected]>

Refactor: Unify EthTx to FilecoinMessage methods

edad5f2

geoff-vball requested a review from a team as a code owner January 21, 2023 05:54

geoff-vball force-pushed the gstuart/unify-tx-hash branch 6 times, most recently from 18d2d97 to f1cbc87 Compare January 21, 2023 10:09

geoff-vball mentioned this pull request Jan 21, 2023

fix: Non-eth transactions can be queried by hash #10026

Closed

7 tasks

geoff-vball force-pushed the gstuart/unify-tx-hash branch 6 times, most recently from 5debd2c to a06827f Compare January 21, 2023 17:44

Filecoin messages can again be converted to Eth Txs

a496300

geoff-vball force-pushed the gstuart/unify-tx-hash branch from a06827f to a496300 Compare January 23, 2023 08:14

arajasek reviewed Jan 23, 2023

View reviewed changes

arajasek mentioned this pull request Jan 23, 2023

Refactor: Unify EthTx to FilecoinMessage methods #10017

Closed

7 tasks

Geoff Stuart added 2 commits January 23, 2023 14:49

All BLS messages should calculated tx hash with unsigned message

dd5f8b0

Refactor newEthTxReceipt

5b3e4ee

geoff-vball force-pushed the gstuart/unify-tx-hash branch from a24ab0e to 534c683 Compare January 23, 2023 20:24

fill in from and to for non-eth transactions

93bab0e

geoff-vball force-pushed the gstuart/unify-tx-hash branch from 534c683 to 93bab0e Compare January 23, 2023 21:32

raulk self-requested a review January 24, 2023 01:35

raulk reviewed Jan 26, 2023

View reviewed changes

Geoff Stuart and others added 2 commits January 27, 2023 15:17

Hoist nil check out of newEthTxFromMessageLookup

7814d72

Merge branch 'release/v1.20.0' into gstuart/unify-tx-hash

f15310a

raulk approved these changes Jan 30, 2023

View reviewed changes

raulk merged commit af72e6f into release/v1.20.0 Jan 30, 2023

raulk deleted the gstuart/unify-tx-hash branch January 30, 2023 18:40

simlecode mentioned this pull request Feb 1, 2023

[venus] merge nv18 consensus / NV18 代码合并 filecoin-project/venus#5550

Closed

jennijuju added this to the Network v18 milestone Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Unify EthTx to FilecoinMessage methods v2 #10095

Refactor: Unify EthTx to FilecoinMessage methods v2 #10095

geoff-vball commented Jan 21, 2023 •

edited

Loading

arajasek Jan 23, 2023

geoff-vball Jan 23, 2023

arajasek Jan 23, 2023

geoff-vball Jan 23, 2023

raulk Jan 26, 2023

geoff-vball Jan 27, 2023

raulk Jan 26, 2023

raulk Jan 26, 2023

geoff-vball Jan 27, 2023

geoff-vball Jan 27, 2023

geoff-vball Jan 27, 2023

raulk left a comment

raulk commented Jan 30, 2023

	func EthTxFromSignedEthMessage(smsg *types.SignedMessage) (EthTx, error) {
	func EthTxFromSignedMessage(smsg *types.SignedMessage) (EthTx, error) {

	func EthTxArgsFromUnsignedEthMessage(msg *types.Message) (EthTxArgs, error) {
	func EthTxArgsFromUnsignedMessage(msg *types.Message) (EthTxArgs, error) {

Refactor: Unify EthTx to FilecoinMessage methods v2 #10095

Refactor: Unify EthTx to FilecoinMessage methods v2 #10095

Conversation

geoff-vball commented Jan 21, 2023 • edited Loading

Related Issues

Proposed Changes

Additional Info

Checklist

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

raulk left a comment

Choose a reason for hiding this comment

raulk commented Jan 30, 2023

geoff-vball commented Jan 21, 2023 •

edited

Loading