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

feat(core)!: define OutputFlags for side-chain contracts #4088

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented May 11, 2022

Description

Defines OutputFlags for side-chain contracts.

Motivation and Context

Define flags required for side-chain contracts. Deprecated flags are kept and can be removed in implementation PRs
for the various side-chain components.

How Has This Been Tested?

Unit and cucumber tests updated as necessary

@sdbondi sdbondi force-pushed the core-output-flags-definition branch 4 times, most recently from 77fc23b to 64033c4 Compare May 11, 2022 06:55
hansieodendaal
hansieodendaal previously approved these changes May 11, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM - barring the CI failures

@mrnaveira
Copy link
Contributor

Same here, LGTM, I believe they're exactly the features we need for the contract lifecycle

/// Output signals validator node acceptance to run a contract.
const CONTRACT_ACCEPT = 0x0400;
/// Output is a contract checkpoint.
const CONTRACT_CHECKPOINT = 0x0800;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need an additional flag for the initial checkpoint where all the contract acceptance UTXOs have to be spent into it - a slight nuance on the rules to be enforced.

Copy link
Contributor

@mrnaveira mrnaveira May 11, 2022

Choose a reason for hiding this comment

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

Isn't that going to be just the first checkpoint? Or does it need to be a custom output for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right and we might need an abandoned flag so the asset owner can spend an abandoned checkpoint into that state to activate the emergency keys

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Member Author

Choose a reason for hiding this comment

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

I left off the INITIAL_CHECKPOINT flag for now because even if we have it, we'd have to check that it is an INITIAL checkpoint (we can't trust the flag), in doing so we have done the work to detect it.

@sdbondi sdbondi force-pushed the core-output-flags-definition branch from 64033c4 to 87ce0ac Compare May 12, 2022 08:24
@sdbondi sdbondi marked this pull request as ready for review May 12, 2022 10:51
@sdbondi sdbondi force-pushed the core-output-flags-definition branch 2 times, most recently from 1a882aa to 3464e21 Compare May 13, 2022 07:34
@sdbondi sdbondi force-pushed the core-output-flags-definition branch from f7e11fa to c05327a Compare May 13, 2022 08:52
@sdbondi sdbondi added the W-consensus_breaking Warn - A change requiring a hard fork to be activated label May 13, 2022
@sdbondi sdbondi changed the title feat(core): define OutputFlags for side-chain contracts feat(core)!: define OutputFlags for side-chain contracts May 13, 2022
@sdbondi sdbondi force-pushed the core-output-flags-definition branch from c05327a to 06f97e6 Compare May 17, 2022 07:18
stringhandler
stringhandler previously approved these changes May 17, 2022
@aviator-app aviator-app bot removed the mq-failed label May 17, 2022
@aviator-app aviator-app bot merged commit 50993a3 into tari-project:development May 17, 2022
@sdbondi sdbondi deleted the core-output-flags-definition branch May 17, 2022 10:07
stringhandler pushed a commit that referenced this pull request Jun 8, 2022
…or consensus enc changes (#4098)

Description
---
- mine a new nonce for `check_difficulty` and `check_share` tests 
- adds `detect_change_in_consensus_encoding` to detect changes in consensus encoding that require updates to pool miner code

Motivation and Context
---
Ref PR #4088 - `check_difficulty` and `check_share` tests are brittle because any change to consensus encoding causes them to fail. In that case, OutputFlags changed from a u8 to a u16. These tests now generate a new nonce and difficulty (takes ~1s on good hardware) and check that the FFI functions return matching results. This was done to eliminate the time and LOC needed to fix these tests in PRs that require changes to consensus encoding (many expected in the coming weeks/months).

`detect_change_in_consensus_encoding` was added to make it clear that code changes in a given PR require an update to the pool miner code.

How Has This Been Tested?
---
The new tests pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-consensus_breaking Warn - A change requiring a hard fork to be activated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants