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

Introduce version and fix various bugs #99

Merged

Conversation

xxuejie
Copy link
Collaborator

@xxuejie xxuejie commented Feb 26, 2020

Since one major use case of CKB VM is on blockchains, we cannot just
change the code in case of bugs. We have to wait for the next fork
event, only then can we upgrade the code. Hence we are introducing a
concept of versions in the VM, whenever we fix a bug that might alter
behavior, we will insert checking code to only include the bug fix in
the next VM version. This way blockchains can expect consistent VM
behavior when they lock specific VM version.

This PR also provides VM version 1 which fixes #92 #97 and #98

pub const OP_SUB: InstructionOpcode = 62;
pub const OP_SUBW: InstructionOpcode = 63;
pub const OP_SW: InstructionOpcode = 64;
pub const OP_VERSION1_JALR: InstructionOpcode = 65;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to allocate different instructon opcode? From the code base, it seems that OP_VERSION1_JALR and OP_JALR lead to identical logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The 2 opcodes actually do use different logic:

.CKB_VM_ASM_LABEL_OP_JALR:
DECODE_I
movq PC_ADDRESS, TEMP1
WRITE_RD(TEMP1)
movq REGISTER_ADDRESS(RS1), TEMP1
addq IMMEDIATE, TEMP1
andq $-2, TEMP1
movq TEMP1, PC_ADDRESS
jmp .prepare_trace

.CKB_VM_ASM_LABEL_OP_VERSION1_JALR:
DECODE_I
movq REGISTER_ADDRESS(RS1), RS2r
movq PC_ADDRESS, TEMP1
WRITE_RD(TEMP1)
addq IMMEDIATE, RS2r
andq $-2, RS2r
movq RS2r, PC_ADDRESS
jmp .prepare_trace

Notice the first 4 lines in each version, they are in different orders. The idea is that version 0 can use the original OP_JALR, while version 1 and later uses OP_VERSION1_JALR

@xxuejie xxuejie force-pushed the introduce-version-and-fix-various-bugs branch from 01cf746 to c4bc509 Compare March 6, 2020 05:00
if version0 {
let address = address.to_u64();
let end = address.checked_add(bytes).ok_or(Error::OutOfBound)?;
if end == RISCV_MAX_MEMORY as u64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am curious why it is not end >= RISCV_MAX_MEMORY

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The > case will be covered by normal memory module bound checking code. This bug only occurs with the == case.

@mohanson
Copy link
Collaborator

If we use develop and release branches to ensure that the mainnet is not broken, do we still need VERSION0 and VERSION1?

@xxuejie
Copy link
Collaborator Author

xxuejie commented Aug 19, 2020

If we use develop and release branches to ensure that the mainnet is not broken, do we still need VERSION0 and VERSION1?

I think we do, based on my understanding of forks, assuming we do a fork update at block 3000000, we will need to use VERSION0 to validate blocks under 3000000 to preserve compatibility, and use VERSION1 to validate blocks after 3000000. That means we will need the concept of version even if we split a release branch.

Since one major use case of CKB VM is on blockchains, we cannot just
change the code in case of bugs. We have to wait for the next fork
event, only then can we upgrade the code. Hence we are introducing a
concept of versions in the VM, whenever we fix a bug that might alter
behavior, we will insert checking code to only include the bug fix in
the next VM version. This way blockchains can expect consistent VM
behavior when they lock specific VM version.
@xxuejie xxuejie force-pushed the introduce-version-and-fix-various-bugs branch from 255729d to 59b076d Compare August 19, 2020 07:26
@doitian
Copy link
Member

doitian commented Aug 25, 2020

If we use develop and release branches to ensure that the mainnet is not broken, do we still need VERSION0 and VERSION1?

I think we do, based on my understanding of forks, assuming we do a fork update at block 3000000, we will need to use VERSION0 to validate blocks under 3000000 to preserve compatibility, and use VERSION1 to validate blocks after 3000000. That means we will need the concept of version even if we split a release branch.

It’s likely that we’ll run v0 for codes stored in cells created before a height, and v1 afterwards. Genesis system scripts are exceptions.

Copy link
Collaborator

@mohanson mohanson left a comment

Choose a reason for hiding this comment

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

LGTM

@xxuejie
Copy link
Collaborator Author

xxuejie commented Aug 27, 2020

It’s likely that we’ll run v0 for codes stored in cells created before a height, and v1 afterwards. Genesis system scripts are exceptions.

This makes sense but I don't think we need to make exceptions for genesis scripts. They can be treated as scripts created before the fork, hence v0 will be used. Later if there is the need to upgrade them, they might be created into new cells, which can leverage v1 or newer versions.

@xxuejie xxuejie merged commit 5e4896e into nervosnetwork:develop Aug 27, 2020
CKB - Pull Requests automation moved this from 👀 Awaiting review to Done Aug 27, 2020
@xxuejie xxuejie deleted the introduce-version-and-fix-various-bugs branch August 27, 2020 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

JALR causes wrong behavior on AsmMachine when rs1 and rd use the same register.
3 participants