Skip to content

rlp: precursor changes for trie, p2p#1778

Merged
obscuren merged 4 commits intoethereum:developfrom
fjl:rlp-trie-changes
Sep 10, 2015
Merged

rlp: precursor changes for trie, p2p#1778
obscuren merged 4 commits intoethereum:developfrom
fjl:rlp-trie-changes

Conversation

@fjl
Copy link
Copy Markdown
Contributor

@fjl fjl commented Sep 7, 2015

These changes are required both for the new trie code and the full rlpx protocol implementation.

@robotally
Copy link
Copy Markdown

robotally commented Sep 7, 2015

Vote Count Reviewers
👍 2 @karalabe @zsfelfoldi
👎 0

Updated: Thu Sep 10 17:26:32 UTC 2015

@fjl fjl changed the title rlp: trie changes rlp: precursor changes for trie, p2p Sep 7, 2015
@fjl fjl added this to the 1.2.0 milestone Sep 7, 2015
@fjl fjl force-pushed the rlp-trie-changes branch 2 times, most recently from 059abe4 to 50c424f Compare September 7, 2015 16:13
@zsfelfoldi
Copy link
Copy Markdown
Contributor

zsfelfoldi commented Sep 7, 2015

👍

karalabe
karalabe reviewed Sep 8, 2015
View reviewed changes
Comment thread rlp/split.go Outdated
Copy link
Copy Markdown
Member

@karalabe karalabe Sep 8, 2015

Choose a reason for hiding this comment

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

Wouldn't this switch be simpler with? (Haven't run the code, just a pseudo-code suggestion)

binary.Read(bytes.NewBuffer(b[:int(slen)]), binary.BigEndian, &s)

Copy link
Copy Markdown
Contributor Author

@fjl fjl Sep 8, 2015

Choose a reason for hiding this comment

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

package binary assumes that the input contains enough bytes to hold a uint64. b isn't always 8 bytes long. Furthermore, the Split* functions should not allocate or use reflection.

Copy link
Copy Markdown
Member

@karalabe karalabe Sep 8, 2015

Choose a reason for hiding this comment

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

Hmmm, fair enough.

@karalabe
Copy link
Copy Markdown
Member

karalabe commented Sep 8, 2015

Could you also add a test which verifies the encoder EOF issue and its resolution? IMHO the bug and fix is very subtle, so I'd like to see a test to avoid an accidental regression.

This was referenced Sep 9, 2015
Merged
Closed
@fjl fjl added in progress and removed review labels Sep 9, 2015
@fjl
Copy link
Copy Markdown
Contributor Author

fjl commented Sep 10, 2015

@karalabe PTAL

@fjl fjl added review and removed in progress labels Sep 10, 2015
@fjl
Copy link
Copy Markdown
Contributor Author

fjl commented Sep 10, 2015

Please don't merge directly after review, I still need to squash these.

karalabe
karalabe reviewed Sep 10, 2015
View reviewed changes
Comment thread rlp/split.go Outdated
Copy link
Copy Markdown
Member

@karalabe karalabe Sep 10, 2015

Choose a reason for hiding this comment

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

The tagsize in this condition took me a while to verify that it cannot screw up. Maybe we could simply write buf[1] there? If it's a string, it means the tag == 1 if length <= 55, or tag == [2-9] if length > 55. Since the content check already made sure we're in the first case, tag must be == 1. It would seem cleaner to write that there, instead of the variable. But this is really nitpicking, I can live without it :D

@karalabe
Copy link
Copy Markdown
Member

karalabe commented Sep 10, 2015

LGTM 👍

@fjl
rlp: fix encReader returning nil buffers to the pool
ac32f52 
The bug can cause crashes if Read is called after EOF has been returned.
No code performs such calls right now, but hitting the bug gets more
likely as rlp.EncodeToReader gets used in more places.
@fjl
Copy link
Copy Markdown
Contributor Author

fjl commented Sep 10, 2015

@karalabe I've moved the check into the case where it applies.

@fjl fjl added the ready label Sep 10, 2015
fjl added 3 commits September 10, 2015 19:41
@fjl
rlp: add Split functions
bc17dba 
These functions allow destructuring of raw rlp-encoded bytes
without the overhead of reflection or copying.
@fjl
rlp: add RawValue
24bb68e
@fjl
rlp: move ListSize to raw.go
fc8b246
@fjl fjl mentioned this pull request Sep 10, 2015
Merged
7 tasks
obscuren added a commit that referenced this pull request Sep 10, 2015
@obscuren
Merge pull request #1778 from fjl/rlp-trie-changes
62bbf8a 
rlp: precursor changes for trie, p2p
@obscuren obscuren merged commit 62bbf8a into ethereum:develop Sep 10, 2015
@obscuren obscuren removed the review label Sep 10, 2015
tony-ricciardi pushed a commit to tony-ricciardi/go-ethereum that referenced this pull request Jan 20, 2022
@piersy @gastonponti
Fix panic in ethstats when block state is unavailable (ethereum#1778)
a0117f5 
* check error from getting statedb for a block to avoid panic

* add a warning log statement

* 🤦

* Update ethstats/ethstats.go

Co-authored-by: piersy <pierspowlesland@gmail.com>

* remove unessasary change

Co-authored-by: piersy <pierspowlesland@gmail.com>
Co-authored-by: Gaston Ponti <pontigaston@gmail.com>
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Aug 4, 2023
@NathanBSC
vote: backup validator sync votes from corresponding mining validator (…
522d4cd 
…ethereum#1778)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@fjl @robotally @zsfelfoldi @karalabe @obscuren