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

Add reserved attribute #2868

Closed
wants to merge 10 commits into from
Closed

Add reserved attribute #2868

wants to merge 10 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Jun 5, 2023

Close #1904

@AnnaShaleva
Copy link
Member

@shargon, are we planning to include this PR into 3.6.0?

@shargon
Copy link
Member Author

shargon commented Jun 5, 2023

@shargon, are we planning to include this PR into 3.6.0?

I'm not in a hurry, except that @roman-khimov or @vncoelho want to

Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Certainly not a blocker for 3.6, but it's good you've updated it anyway.

/// Indicates that the transaction is a reserved attribute.
/// </summary>
[ReflectionCache(typeof(ReservedAttribute))]
ReservedAttribute = 0x31
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the idea was to have a range of attributes (suppose you're creating an alternative to #1573 and defining 10 attributes instead of 3). And it's better be some upper range to reduce collisions with "regular" type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like in #1904 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "Range of Attributes"?

In the way it is implemented we can setup a reserved byte that have ff possibilities as range for the dapp, right @shargon ?

@vncoelho
Copy link
Member

vncoelho commented Jun 5, 2023

Not in hurry, @shargon.

But this do not have any possible problem, does it have?
What if some transactions in the past used this attribute ID? Do we need to hard fork something?

@vncoelho
Copy link
Member

vncoelho commented Jun 5, 2023

            foreach (TransactionAttribute attribute in Attributes)
                if (!attribute.Verify(snapshot, this))
                    return VerifyResult.InvalidAttribute;

https://github.com/neo-project/neo/blob/535aa2057b6ddddb90b2009ef47f8ba16fd3d036/src/Neo/Network/P2P/Payloads/Transaction.cs#LL366C1-L368C58

@Jim8y Jim8y added the Need Active Pr will be closed after one week if no new activity. label Feb 12, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Feb 18, 2024

Close as inactive

@Jim8y Jim8y closed this Feb 18, 2024
@shargon shargon deleted the reserve-attr branch February 18, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Active Pr will be closed after one week if no new activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reserve some TransactionAttributeTypes for experimental/private usage
5 participants