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 timestamp field from block header #2225

Closed
noamnelke opened this issue Dec 7, 2020 · 8 comments
Closed

Remove timestamp field from block header #2225

noamnelke opened this issue Dec 7, 2020 · 8 comments
Assignees
@krishnaindani
Labels
good first issue help wanted technical debt

Comments

@noamnelke
Copy link
Member

Description

The block header contains a Timestamp field, which is set, but never read.

It’s problematic to use it anywhere since it’s un-vetted, one can plaster any timestamp that serves them onto anything.
Since we’re not doing anything with this field we may as well call it “arbitrary 8 bytes that the miner can fill with whatever they please”.

Affected code

go-spacemesh/common/types/block.go

Lines 124 to 133 in f76c8b9

type BlockHeader struct {
LayerIndex LayerID
ATXID ATXID
EligibilityProof BlockEligibilityProof
Data []byte
Coin bool
Timestamp int64
BlockVotes []BlockID
ViewEdges []BlockID
}

@avive
Copy link
Contributor

avive commented Dec 8, 2020

why not remove it and save some bytes per stored tx?

@krishnaindani
Copy link
Contributor

Hi, Is this issue available? If so I would like to try working on this.

@noamnelke
Copy link
Member Author

@avive Removing the field is what this issue is about, see title.

@krishnaindani Sure! Feel free to submit a pull request.

@krishnaindani
Copy link
Contributor

Thanks @noamnelke . I will get started and submit the pr

@krishnaindani
Copy link
Contributor

I see the block header defined here and used here. I only see these two places to remove the field. Am I understanding correctly?

@noamnelke
Copy link
Member Author

Yes, looks that way.

@krishnaindani krishnaindani mentioned this issue Dec 10, 2020
Closed
3 tasks
@krishnaindani
Copy link
Contributor

Yes, looks that way.

Just wanted to cross check. Thanks. I have created the pr.

@krishnaindani
Copy link
Contributor

I have added a PR for this issue. Can someone check if the PR looks good or if I need to update anything there? Thanks in advance.

bors bot pushed a commit that referenced this issue Dec 17, 2020
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
729de44 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Dec 17, 2020
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
616e044 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Dec 17, 2020
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
d50c603 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Jan 4, 2021
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
19eeae2 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Jan 4, 2021
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
ea76cb9 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Jan 4, 2021
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
4a2844d 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Jan 5, 2021
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
2909089 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
bors bot pushed a commit that referenced this issue Jan 5, 2021
@krishnaindani
Krishna/2225/remove timestamp from block header (#2231)
79700fb 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #2225
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
Remove Timestamp field from the BlockHeader struct 

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)(#2225)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed
@noamnelke noamnelke mentioned this issue Mar 22, 2021
Closed
4 tasks
@bors bors bot closed this as completed in 573e0bb Mar 25, 2021
lrettig added a commit that referenced this issue Mar 30, 2021
@krishnaindani @noamnelke @lrettig
Remove timestamp (#2343)
aa43f3a 
## Motivation
Closes #2225 (obsoletes #2231)

## Changes
Remove `Timestamp` field from the `BlockHeader` struct.

## Test Plan
Unit tests have been updated and all pass consistently. System tests should be unaffected and will be run.

## DevOps Notes
- [x] Does this code require configuration changes? ❌ 
- [x] Does this code affect public APIs? ❌ 
- [x] Does this code assume a new version of external services (PoET, elasticsearch, etc.) ❌ 
- [x] Does this code make changes to log messages that our monitoring infrastructure may rely on? ❌ 

Co-authored-by: Noam Nelke <noamnelke@gmail.com>
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
lrettig added a commit that referenced this issue Mar 30, 2021
@krishnaindani @noamnelke @lrettig
Remove timestamp (#2343)
aeae1b1 
## Motivation
Closes #2225 (obsoletes #2231)

## Changes
Remove `Timestamp` field from the `BlockHeader` struct.

## Test Plan
Unit tests have been updated and all pass consistently. System tests should be unaffected and will be run.

## DevOps Notes
- [x] Does this code require configuration changes? ❌ 
- [x] Does this code affect public APIs? ❌ 
- [x] Does this code assume a new version of external services (PoET, elasticsearch, etc.) ❌ 
- [x] Does this code make changes to log messages that our monitoring infrastructure may rely on? ❌ 

Co-authored-by: Noam Nelke <noamnelke@gmail.com>
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
lrettig added a commit that referenced this issue Mar 30, 2021
@krishnaindani @noamnelke @lrettig
Remove timestamp (#2343)
5ba46b1 
## Motivation
Closes #2225 (obsoletes #2231)

## Changes
Remove `Timestamp` field from the `BlockHeader` struct.

## Test Plan
Unit tests have been updated and all pass consistently. System tests should be unaffected and will be run.

## DevOps Notes
- [x] Does this code require configuration changes? ❌ 
- [x] Does this code affect public APIs? ❌ 
- [x] Does this code assume a new version of external services (PoET, elasticsearch, etc.) ❌ 
- [x] Does this code make changes to log messages that our monitoring infrastructure may rely on? ❌ 

Co-authored-by: Noam Nelke <noamnelke@gmail.com>
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
lrettig added a commit that referenced this issue Mar 31, 2021
@krishnaindani @noamnelke @lrettig
Remove timestamp (#2343)
c2c087c 
## Motivation
Closes #2225 (obsoletes #2231)

## Changes
Remove `Timestamp` field from the `BlockHeader` struct.

## Test Plan
Unit tests have been updated and all pass consistently. System tests should be unaffected and will be run.

## DevOps Notes
- [x] Does this code require configuration changes? ❌ 
- [x] Does this code affect public APIs? ❌ 
- [x] Does this code assume a new version of external services (PoET, elasticsearch, etc.) ❌ 
- [x] Does this code make changes to log messages that our monitoring infrastructure may rely on? ❌ 

Co-authored-by: Noam Nelke <noamnelke@gmail.com>
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
lrettig added a commit that referenced this issue Apr 1, 2021
@krishnaindani @noamnelke @lrettig
Remove timestamp (#2343)
8975537 
## Motivation
Closes #2225 (obsoletes #2231)

## Changes
Remove `Timestamp` field from the `BlockHeader` struct.

## Test Plan
Unit tests have been updated and all pass consistently. System tests should be unaffected and will be run.

## DevOps Notes
- [x] Does this code require configuration changes? ❌ 
- [x] Does this code affect public APIs? ❌ 
- [x] Does this code assume a new version of external services (PoET, elasticsearch, etc.) ❌ 
- [x] Does this code make changes to log messages that our monitoring infrastructure may rely on? ❌ 

Co-authored-by: Noam Nelke <noamnelke@gmail.com>
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@krishnaindani krishnaindani

Labels
good first issue help wanted technical debt
Projects
None yet
Milestone
No milestone
3 participants
@avive @noamnelke @krishnaindani