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.

Sign up for GitHub

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

Jump to bottom

Remove Metadata from Output features [for non coinbase outputs] #4908

Closed
stringhandler opened this issue Nov 11, 2022 · 4 comments
Closed

Remove Metadata from Output features [for non coinbase outputs] #4908

stringhandler opened this issue Nov 11, 2022 · 4 comments
Assignees
@agubarev
Labels
A-base_node Area - The Tari base node executable and libraries A-transaction_sending Area - related to sending transactions C-tech_debt tidies up technical debt
Milestone
Stagenet Freeze

Comments

@stringhandler
Copy link
Collaborator

Metadata is a remnant of the previous DAN implementation. It should be removed from output features
@stringhandler stringhandler added C-tech_debt tidies up technical debt A-base_node Area - The Tari base node executable and libraries A-transaction_sending Area - related to sending transactions labels Nov 11, 2022
@stringhandler stringhandler added this to the Stagenet Freeze milestone Nov 11, 2022
@stringhandler
Copy link
Collaborator Author

it might be useful to allow this field only for coinbases, and store information here to display the mining pool that mined this block. Showing the mining pool will make it useful to see if a mining pool has more than 30 or 51%

@agubarev agubarev self-assigned this Nov 21, 2022
@stringhandler
Copy link
Collaborator Author

I think the rule should be: This field should be optional for coinbases (mining pools and other merge mined coins can use it), but it should be empty for non-coinbases.

For coinbases, the maximum length should be 64 bytes (2x hashes), so that arbitrary data cannot be included

@stringhandler stringhandler changed the title Remove Metadata from Output features Remove Metadata from Output features [for non coinbase outputs] Nov 23, 2022
This was referenced Nov 25, 2022
Closed
Merged
stringhandler pushed a commit that referenced this issue Nov 25, 2022
@agubarev
feat: only coinbase output features may have metadata set, and is of …
22b1330 
…limited size; ref #4908 (#4960)

Description
---
Added validation to OutputFeatures which, for now, makes sure that only coinbase output features may have metadata set.

Motivation and Context
---
Remove Metadata from Output features: metadata is a remnant of the previous DAN implementation. It should be removed from output features

#4908

How Has This Been Tested?
---
unit tests
agubarev added a commit that referenced this issue Nov 25, 2022
@agubarev
Revert "feat: only coinbase output features may have metadata set, an…
31bbfc0 
…d is of limited size; ref #4908 (#4960)"

This reverts commit 22b1330.
@agubarev
Copy link
Contributor

Although merged, I should make another PR to reflect Stan's comment, PR got merged before I managed to push an update. Will address to this ticket.

@agubarev
Copy link
Contributor

agubarev commented Dec 1, 2022

On second thought, satisfying the aforementioned performance nitpick (on the PR), at the moment, would create unnecessary indirection in the code. Perhaps this can be addressed more properly later, via larger optimization.

@agubarev agubarev closed this as completed Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@agubarev agubarev

Labels
A-base_node Area - The Tari base node executable and libraries A-transaction_sending Area - related to sending transactions C-tech_debt tidies up technical debt
Projects
Tari Esme Testnet
Archived in project
Milestone
Stagenet Freeze
Development

No branches or pull requests
2 participants
@agubarev @stringhandler