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

consensus: added partial message decoding #859

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

AnnaShaleva
Copy link
Member

closes #849

@AnnaShaleva AnnaShaleva added consensus dBFT consensus neo3 labels Apr 15, 2020
@AnnaShaleva AnnaShaleva self-assigned this Apr 15, 2020
@AnnaShaleva AnnaShaleva force-pushed the neo3/partial_consensus_message_decoding branch from e5568f3 to bb991b2 Compare April 15, 2020 16:04
ww := io.NewBufBinWriter()
p.message.EncodeBinary(ww.BinWriter)
w.WriteVarBytes(ww.Bytes())
w.WriteVarBytes(p.data)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like

if p.message != nil {
    ww := io.NewBufBinWriter()
    p.message.EncodeBinary(ww.BinWriter)
    p.data = ww.Bytes()
}
w.WriteVarBytes(p.data)

And then you don't need to update p.data in every Set method.

@AnnaShaleva AnnaShaleva force-pushed the neo3/partial_consensus_message_decoding branch 4 times, most recently from 08a0857 to 8d31e26 Compare April 16, 2020 07:38
@AnnaShaleva AnnaShaleva marked this pull request as ready for review April 16, 2020 07:42
@AnnaShaleva AnnaShaleva force-pushed the neo3/partial_consensus_message_decoding branch from 8d31e26 to 3d4a1d4 Compare April 16, 2020 09:07
@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #859 into master will increase coverage by 0.00%.
The diff coverage is 78.94%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #859   +/-   ##
=======================================
  Coverage   63.43%   63.43%           
=======================================
  Files         183      183           
  Lines       15733    15744   +11     
=======================================
+ Hits         9980     9987    +7     
- Misses       5272     5275    +3     
- Partials      481      482    +1     
Impacted Files Coverage Δ
pkg/consensus/consensus.go 39.61% <0.00%> (-0.62%) ⬇️
pkg/consensus/payload.go 97.43% <100.00%> (+0.12%) ⬆️
pkg/consensus/recovery_message.go 84.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e3c364...9dd5ab5. Read the comment docs.

pkg/consensus/payload.go Outdated Show resolved Hide resolved
@AnnaShaleva AnnaShaleva force-pushed the neo3/partial_consensus_message_decoding branch 2 times, most recently from fd6eb87 to b83af55 Compare April 16, 2020 09:19
@roman-khimov
Copy link
Member

We also need to fix dbft.WithNewConsensusPayload() invocation.

@AnnaShaleva AnnaShaleva force-pushed the neo3/partial_consensus_message_decoding branch 2 times, most recently from aa1f803 to e0d13b6 Compare April 16, 2020 09:27
@AnnaShaleva
Copy link
Member Author

We also need to fix dbft.WithNewConsensusPayload() invocation.

Did you mean

dbft.WithNewConsensusPayload(func() payload.ConsensusPayload { p := new(Payload); p.message = &message{}; return p }),

?

@roman-khimov
Copy link
Member

Yep, p.message was obviously uninitialized there and dbft library will use p.message for its purposes.

@AnnaShaleva AnnaShaleva force-pushed the neo3/partial_consensus_message_decoding branch from e0d13b6 to 9dd5ab5 Compare April 16, 2020 11:36
@roman-khimov roman-khimov merged commit e150bd7 into master Apr 16, 2020
@roman-khimov roman-khimov deleted the neo3/partial_consensus_message_decoding branch April 16, 2020 13:59
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus dBFT consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial consensus message decoding for non-CN nodes
4 participants