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

Conditional CU metering #1

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conditional CU metering #1

wants to merge 16 commits into from

Conversation

tao-stones
Copy link
Owner

No description provided.

@tao-stones tao-stones force-pushed the conditional-cu-charging branch from a3ee270 to 55e4924 Compare September 26, 2024 19:57
let execution_cu = match transaction.execution_results {
Ok(_) || Err(TransactionError::CustomError(_)) => committed_tx.executed_cu,
_ => transaction.requested_cu,
};
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not too familiar with InstructionError, wondering if correct to use Requested_cu for any error but TransactionError::CustomError(_)

Choose a reason for hiding this comment

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

This is the most important part to get right. In Agave's VM, I think it's

EbpfError::DivideByZero
EbpfError::DivideOverflow
EbpfError::CallOutsideTextSegment
EbpfError::InvalidInstruction
EbpfError::InvalidVirtualAddress

I'm not sure exactly what these translate to as TransactionErrors though.
In firedancer, it's maybe something like

#define FD_VM_ERR_SIGSPLIT    ( -9) /* split multiword instruction (e.g. jump into the middle of a multiword instruction) */
#define FD_VM_ERR_SIGILL      (-12) /* illegal instruction (e.g. opcode is not valid) */
#define FD_VM_ERR_SIGSEGV     (-13) /* illegal memory address (e.g. read/write to an address not backed by any memory) */
#define FD_VM_ERR_SIGBUS      (-14) /* misaligned memory address (e.g. read/write to an address with inappropriate alignment) */
#define FD_VM_ERR_SIGRDONLY   (-15) /* illegal write (e.g. write to a read only address) */
#define FD_VM_ERR_SIGFPE      (-18) /* divide by zero */

Copy link
Owner Author

Choose a reason for hiding this comment

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

indeed, what about AccessViolation in agave's VM, should it also be considered as "irregular failure"?

I failed to find where EbpfError converted to InstructionError, there are some casting at bpf_loader, is it the right place to look at @Lichtso ?

Copy link

Choose a reason for hiding this comment

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

Copy link

@ptaffet-jump ptaffet-jump left a comment

Choose a reason for hiding this comment

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

Started reviewing. Here's some feedback. Feel free to adjust it as necessary.

let execution_cu = match transaction.execution_results {
Ok(_) || Err(TransactionError::CustomError(_)) => committed_tx.executed_cu,
_ => transaction.requested_cu,
};

Choose a reason for hiding this comment

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

This is the most important part to get right. In Agave's VM, I think it's

EbpfError::DivideByZero
EbpfError::DivideOverflow
EbpfError::CallOutsideTextSegment
EbpfError::InvalidInstruction
EbpfError::InvalidVirtualAddress

I'm not sure exactly what these translate to as TransactionErrors though.
In firedancer, it's maybe something like

#define FD_VM_ERR_SIGSPLIT    ( -9) /* split multiword instruction (e.g. jump into the middle of a multiword instruction) */
#define FD_VM_ERR_SIGILL      (-12) /* illegal instruction (e.g. opcode is not valid) */
#define FD_VM_ERR_SIGSEGV     (-13) /* illegal memory address (e.g. read/write to an address not backed by any memory) */
#define FD_VM_ERR_SIGBUS      (-14) /* misaligned memory address (e.g. read/write to an address with inappropriate alignment) */
#define FD_VM_ERR_SIGRDONLY   (-15) /* illegal write (e.g. write to a read only address) */
#define FD_VM_ERR_SIGFPE      (-18) /* divide by zero */

Ok(_) || Err(TransactionError::CustomError(_)) => committed_tx.executed_cu,
_ => transaction.requested_cu,
};
... ...
Copy link
Owner Author

Choose a reason for hiding this comment

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

alternatively, instead of charging different CUs based on TransactionError at call sites, we could make VM to consume CUs differently - deplete CU meter in case of "irregular failure"; so the call sites (banking stage or replay) don't need to change. I thought this is what @Lichtso implied somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants