Skip to content
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

account_code and account_code_size host I/Os #191

Merged
merged 8 commits into from
Jan 30, 2024
Merged

account_code and account_code_size host I/Os #191

merged 8 commits into from
Jan 30, 2024

Conversation

Copy link

cla-bot bot commented Jan 5, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

2 similar comments
Copy link

cla-bot bot commented Jan 5, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Copy link

cla-bot bot commented Jan 5, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Copy link

cla-bot bot commented Jan 9, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Copy link

cla-bot bot commented Jan 9, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Copy link

cla-bot bot commented Jan 9, 2024

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 56 lines in your changes are missing coverage. Please review.

Comparison is base (c817c85) 55.07% compared to head (a2d9bb1) 38.04%.
Report is 1 commits behind head on stylus.

❗ Current head a2d9bb1 differs from pull request most recent head 5c9f1ee. Consider uploading reports for the commit 5c9f1ee to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           stylus     #191       +/-   ##
===========================================
- Coverage   55.07%   38.04%   -17.04%     
===========================================
  Files         222      282       +60     
  Lines       31219    43430    +12211     
===========================================
- Hits        17195    16521      -674     
- Misses      11552    25448    +13896     
+ Partials     2472     1461     -1011     

Copy link
Contributor

@rachel-bousfield rachel-bousfield left a comment

Choose a reason for hiding this comment

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

Nice work! Some changes we'll need to make for the economics and UX of these affordances

arbitrator/wasm-libraries/user-host-trait/src/lib.rs Outdated Show resolved Hide resolved
arbitrator/arbutil/src/evm/api.rs Outdated Show resolved Hide resolved
arbitrator/stylus/tests/evm-data/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 480 to 487
fn account_code(&mut self, address: u32, dest: u32, size: u32) -> Result<(), Self::Err> {
self.buy_ink(HOSTIO_INK + EVM_API_INK)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also require enough gas for size bytes and pass size to Geth, which should only copy up to that many

Comment on lines 492 to 506
fn account_code_size(&mut self, address: u32) -> Result<u32, Self::Err> {
self.buy_ink(HOSTIO_INK + EVM_API_INK)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably require a sentry for these or at the very least params.ExtcodeCopyBaseEIP150

Copy link
Author

Choose a reason for hiding this comment

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

I don't know what "sentry" means in this context.

Copy link
Contributor

Choose a reason for hiding this comment

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

In Ethereum a sentry is an amount of gas you must have, but needn't pay, to do something. You can use require_ink and require_gas to achieve this. It's useful in contexts where you're about to do work that might be expensive, or if you want to restrict the kinds of things one can do with a call stipend (another EVM concept: a small gas credit you get when transferring value but for which we only want nonmutating purchases)

Comment on lines 275 to 272
cost := vm.WasmAccountTouchCost(evm.StateDB, address)
if !evm.StateDB.Empty(address) {
return evm.StateDB.GetCode(address), cost
}
return []byte{}, cost
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't charge a constant amount of gas for a linear amount of work.

Also no need to check if the account is empty (see vm.opExtCodeCopy in instructions.go)


metaCost := vm.WasmAccountTouchCost(evm.StateDB, address)
loadCost := 3 * am.WordsForBytes(params.MaxCodeSize)
sentry := am.SaturatingUAdd(metaCost, loadCost)
Copy link
Author

Choose a reason for hiding this comment

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

Is a saturating add really what is needed here? Wouldn't it actually be a problem if this overflowed?

//export accountCodeHashImpl
func accountCodeHashImpl(api usize, address bytes20, cost *u64) bytes32 {
//export accountCodeSizeImpl
func accountCodeSizeImpl(api usize, address bytes20, cost *u64) u32 {
Copy link
Author

Choose a reason for hiding this comment

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

This interface seems not quite ideal to me (and where a similar interface appears for other functions), since the cost parameter is used both to input the current amount of gas and to output the cost - why not two different parameters?

Copy link
Contributor

@rachel-bousfield rachel-bousfield left a comment

Choose a reason for hiding this comment

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

Looks great!

@rachel-bousfield rachel-bousfield merged commit d4e30af into stylus Jan 30, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants