FAB-15817 Prefer last config in signatures metadata field #363
Conversation
wlahti
requested review from
ale-linux,
binhn,
C0rWin,
christo4ferris,
denyeart,
guoger,
jyellick,
kchristidis,
manish-sethi,
mastersingh24,
muralisrini,
sykesm and
yacovm
as code owners
|
/ci-run |
|
AZP build triggered! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
wlahti
force-pushed
the
FAB-15817
branch
2 times, most recently
from
adaecf1
to
8457066
Compare
|
/ci-run |
|
AZP build triggered! |
|
/ci-run |
|
AZP build triggered! |
|
/ci-run |
|
AZP build triggered! |
|
/ci-run |
|
AZP build triggered! |
|
/ci-run |
|
AZP build already running! |
|
/ci-run |
|
AZP build triggered! |
There was a problem hiding this comment.
We need to make sure that:
a) Old clients can still use new blockchains (ie, continue to populate the old metadata position)
b) New clients can still parse old blockchains (ie, fall-back to the old metadata position if the new one is empty)
c) New clients prefer the new metadata location (the old location is not signed)
common/ledger/blockledger/util.go
Outdated
| @@ -74,6 +78,11 @@ func CreateNextBlock(rl Reader, messages []*cb.Envelope) *cb.Block { | |||
| block := protoutil.NewBlock(nextBlockNumber, previousBlockHash) | |||
| block.Header.DataHash = protoutil.BlockDataHash(data) | |||
| block.Data = data | |||
| block.Metadata.Metadata[cb.BlockMetadataIndex_SIGNATURES] = protoutil.MarshalOrPanic(&cb.Metadata{ | |||
There was a problem hiding this comment.
I'm curious why we're populating this metadata field here now, when we didn't use to?
There was a problem hiding this comment.
It's due to the LastConfig now being nested inside the OrdererBlockMetadata instead of as its own separate value. Without setting this here, certain tests panic with a nil pointer when attempting to retrieve the index of a nil LastConfig (in regstrar_test.go's testLastConfigBlockNumber function). It's possibly the case that these tests weren't really testing that the last config was set correctly and merely relied on the default value.
There was a problem hiding this comment.
Then I think we need to fix those tests. The CreateNextBlock function has no business setting this metadata. In fact, it can't set it correctly, because it doesn't know whether this block is a config block (in which case, the last config should be this block number.) As implemented above, all blocks would point to block 0 as the last config, as there is no case where the value is updated.
wlahti
changed the title
common/ledger/blockledger/util.go
Outdated
| @@ -74,6 +78,11 @@ func CreateNextBlock(rl Reader, messages []*cb.Envelope) *cb.Block { | |||
| block := protoutil.NewBlock(nextBlockNumber, previousBlockHash) | |||
| block.Header.DataHash = protoutil.BlockDataHash(data) | |||
| block.Data = data | |||
| block.Metadata.Metadata[cb.BlockMetadataIndex_SIGNATURES] = protoutil.MarshalOrPanic(&cb.Metadata{ | |||
There was a problem hiding this comment.
Then I think we need to fix those tests. The CreateNextBlock function has no business setting this metadata. In fact, it can't set it correctly, because it doesn't know whether this block is a config block (in which case, the last config should be this block number.) As implemented above, all blocks would point to block 0 as the last config, as there is no case where the value is updated.
orderer/common/cluster/util.go
Outdated
| @@ -652,7 +652,7 @@ func LastConfigBlock(block *common.Block, blockRetriever BlockRetriever) (*commo | |||
| if blockRetriever == nil { | |||
| return nil, errors.New("nil blockRetriever") | |||
| } | |||
| if block.Metadata == nil || len(block.Metadata.Metadata) <= int(common.BlockMetadataIndex_LAST_CONFIG) { | |||
| if block.Metadata == nil || len(block.Metadata.Metadata) <= int(common.BlockMetadataIndex_SIGNATURES) { | |||
There was a problem hiding this comment.
Either this check is not helping us, or we could have a similar out of bounds reference here.
wlahti
force-pushed
the
FAB-15817
branch
2 times, most recently
from
9d52978
to
fd5feee
Compare
protoutil/blockutils.go
Outdated
| @@ -130,7 +138,7 @@ func GetMetadataFromBlockOrPanic(block *cb.Block, index cb.BlockMetadataIndex) * | |||
| func GetConsenterMetadataFromBlock(block *cb.Block) (*cb.Metadata, error) { | |||
| m, err := GetMetadataFromBlock(block, cb.BlockMetadataIndex_SIGNATURES) | |||
| if err != nil { | |||
| return nil, errors.Wrapf(err, "failed to retrieve metadata at index: %d", cb.BlockMetadataIndex_SIGNATURES) | |||
| return nil, errors.Wrap(err, "failed to retrieve metadata") | |||
There was a problem hiding this comment.
If you're changing this, let's do a WithMessage instead of a Wrap, no need to have two stacks attached.
|
/ci-run |
|
AZP build triggered! |
This commit updates the orderer and peer to prefer retrieving the last config from the SIGNATURES field and fallback to retrieving it from the LAST_CONFIG field. Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
This commit updates the orderer and peer to prefer retrieving the
last config from the SIGNATURES field and fallback to retrieving
it from the LAST_CONFIG field.
Signed-off-by: Will Lahti wtlahti@us.ibm.com