Skip to content

Fix virtual channel proposal id generation#300

Merged
matthiasgeihs merged 2 commits intohyperledger-labs:mainfrom
boschresearch:299-correct-virt-ch-proposal-id
Jan 20, 2022
Merged

Fix virtual channel proposal id generation#300
matthiasgeihs merged 2 commits intohyperledger-labs:mainfrom
boschresearch:299-correct-virt-ch-proposal-id

Conversation

@manoranjith
Copy link
Copy Markdown

@manoranjith manoranjith commented Jan 19, 2022

  • Previously, the proposal ID for a virtual channel was generated using
    the ProposalID method defined on BaseChannelProposal.

  • Hence, not all parameters of the virtual channel proposal were used.

  • Fix it by defining a ProposalID method on the virtual channel.

  • Also, remove the ProposalID method on the BaseChannelProposal, as it
    is not needed.

Fixes #299.

@manoranjith manoranjith changed the title 299 correct virt ch proposal id Fix virtual channel proposal id generation Jan 19, 2022
@manoranjith manoranjith force-pushed the 299-correct-virt-ch-proposal-id branch 2 times, most recently from 03455f8 to 1d2e587 Compare January 19, 2022 07:47
@manoranjith manoranjith force-pushed the 299-correct-virt-ch-proposal-id branch 2 times, most recently from 9407242 to f162df9 Compare January 19, 2022 08:25
Copy link
Copy Markdown
Contributor

@matthiasgeihs matthiasgeihs left a comment

Choose a reason for hiding this comment

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

I think we can achieve the same thing by using VirtualChannelProposal.Encode.

Comment thread client/proposalmsgs.go
Comment on lines +612 to +613
// ProposalID returns the identifier of this channel proposal.
func (p VirtualChannelProposal) ProposalID() (propID ProposalID) {
hasher := newHasher()
if err := perunio.Encode(hasher,
p.Base(),
p.Proposer,
wire.Addresses(p.Peers),
); err != nil {
log.Panicf("proposal ID base encoding: %v", err)
}
for i := range p.Parents {
if err := perunio.Encode(hasher, p.Parents[i]); err != nil {
log.Panicf("proposal ID base encoding: %v", err)
}
}
for i := range p.IndexMaps {
for j := range p.IndexMaps {
if err := perunio.Encode(hasher, (p.IndexMaps[i][j])); err != nil {
log.Panicf("proposal ID base encoding: %v", err)
}
}
}

copy(propID[:], hasher.Sum(nil))
return
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can reuse the VirtualChannelProposal.Encode method:

Suggested change
// ProposalID returns the identifier of this channel proposal.
func (p VirtualChannelProposal) ProposalID() (propID ProposalID) {
hasher := newHasher()
if err := perunio.Encode(hasher,
p.Base(),
p.Proposer,
wire.Addresses(p.Peers),
); err != nil {
log.Panicf("proposal ID base encoding: %v", err)
}
for i := range p.Parents {
if err := perunio.Encode(hasher, p.Parents[i]); err != nil {
log.Panicf("proposal ID base encoding: %v", err)
}
}
for i := range p.IndexMaps {
for j := range p.IndexMaps {
if err := perunio.Encode(hasher, (p.IndexMaps[i][j])); err != nil {
log.Panicf("proposal ID base encoding: %v", err)
}
}
}
copy(propID[:], hasher.Sum(nil))
return
}
// ProposalID returns the identifier of this channel proposal.
func (p VirtualChannelProposal) ProposalID() (propID ProposalID) {
hasher := newHasher()
if err := perunio.Encode(hasher, p); err != nil {
log.Panicf("encoding proposal: %v", err)
}
copy(propID[:], hasher.Sum(nil))
return
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, I agree.

Copy link
Copy Markdown
Author

@manoranjith manoranjith Jan 19, 2022

Choose a reason for hiding this comment

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

Updated in 248ecfc. Did the same for ledger channel and sub channel proposals, in the following commit: 2b3f79e.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As it was the only suggestion, I squashed the review commit and rebased the branch onto develop.

@manoranjith manoranjith dismissed a stale review via e5f0d02 January 19, 2022 10:35
@manoranjith manoranjith force-pushed the 299-correct-virt-ch-proposal-id branch 2 times, most recently from e5f0d02 to 4576ba2 Compare January 19, 2022 10:39
Manoranjith added 2 commits January 19, 2022 16:14
- Previously, the proposal ID for a virtual channel was generated using
  the ProposalID method defined on BaseChannelProposal.

- Hence, not all parameters of the virtual channel proposal were used.

- Fix it by defining a ProposalID method on the virtual channel.

- Also, remove the ProposalID method on the BaseChannelProposal, as it
  is not needed.

Signed-off-by: Manoranjith <ponraj.manoranjitha@in.bosch.com>
- Use the Encode method for encoding the proposal, before hashing it.

Signed-off-by: Manoranjith <ponraj.manoranjitha@in.bosch.com>
@manoranjith manoranjith force-pushed the 299-correct-virt-ch-proposal-id branch from 4576ba2 to 2b3f79e Compare January 19, 2022 10:44
@matthiasgeihs matthiasgeihs merged commit d960e1c into hyperledger-labs:main Jan 20, 2022
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.

Correctly generate the ID for virtual channel proposal

2 participants