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

UIP-0027 - Independent service fields for transaction #21

Closed
wants to merge 2 commits into from

Conversation

dsaveliev
Copy link
Member

The given UIP-0027 describes the way to reimplement type and version transaction fields with a little pain.

Related to:
dtr-org/unit-e#1011
dtr-org/unit-e#1062

Signed-off-by: Dmitry Saveliev [email protected]

Signed-off-by: Dmitry Saveliev <[email protected]>
@dsaveliev dsaveliev requested review from scravy and a team May 7, 2019 13:12
@dsaveliev dsaveliev self-assigned this May 7, 2019
UIP-0027.md Show resolved Hide resolved
@cmihai cmihai requested a review from a team May 7, 2019 13:22
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
UIP-0027.md Outdated Show resolved Hide resolved
* `version` - independent`uint8_t` field


## Rationale
Copy link
Member

@frolosofsky frolosofsky May 10, 2019

Choose a reason for hiding this comment

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

In both Rationale and Motivation parts, I did not find a description of a real problem we're trying to solve. The essential proposed change is literally to shrink version from 16 bits to 8 bits that leads to having two dead bytes instead of one.

Personally, I'd rather replace nVersion (4 bytes) with two fields: version and txtype (each 1 byte). As for bitcoin tests which relies on "real" transactions, I think we can get rid of them in favor of new tests for unit-e transactions. Having this compatibility with bitcoin limits us in further transactions manipulations.

If we say that it's too hard to achieve and maintain (which is understandable), then I don't see a point in this UIP. We can pinch unused byte of nVersion anytime we need, as we already did it in UIP-18.

@Gnappuraz
Copy link
Member

I don't wanna belittle the proposal, but it honestly does not seem to justify the amount of changes probably required. I understand that cleaning it up is nice, but until we actually need those extra bytes I don't see a big problem with the bit-masking we have right now.

@dsaveliev
Copy link
Member Author

dsaveliev commented May 10, 2019

Well, if you'll take a look at dtr-org/unit-e#1062, you'll see my opinion. @scravy WDYT?

@scravy
Copy link
Member

scravy commented May 13, 2019

Transaction Type was already changed from 16 to 8 bits in dtr-org/unit-e#1004. It did not come with a UIP or anything, and introduced a dead byte which lead to the creation of dtr-org/unit-e#1011

I for one would drop version entirely, as expressed in that ticket, but #1062 shows that that's a lot of changes which make it harder to upstream sync. The suggested solution was to clean it up in the sense that there would be four explicit fields of which 2 are reserved for future changes, which I for one think is a good compromise.

@dsaveliev dsaveliev closed this Mar 22, 2023
@dsaveliev dsaveliev deleted the UIP-0027 branch March 22, 2023 12:27
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.

9 participants