[FAB-17039] Skip retrieving pvtdata from transient store if txid is empty #2183
Conversation
wenjianqiao
force-pushed
the
datatriever_no_txid
branch
from
a73df76
to
a3a6bdc
Compare
wenjianqiao
changed the title
gossip/privdata/dataretriever.go
Outdated
| results := make(Dig2PvtRWSetWithConfig) | ||
| for _, dig := range digests { | ||
| // skip retrieving from transient store for reconciliation requests | ||
| if isReconciliationRequest(dig) { |
There was a problem hiding this comment.
Though this is correct that txID is not included in the case of reconciliation at the caller side, but drawing this conclusion at this function which the the sender side, breaks the principal of isolation. I find previous patch more suitable here that if caller does not supply txID (for whatever reason), skip looking in the transient store. We can certainly write a comment when this can happen but the caller code is free to change in general. For instance, caller is free not to send txID even in the case of commit path. In fact, commit path/ reconciliation terms are meaningful only at the caller side. This place is just getting asked for data either by txID, or by blockNum:TxNum (or both).
Also, having this if condition in a loop, may give someone a wrong impression that the requests for reconciliation and commit path gets combined on the caller side.
@ale-linux - second thought?
There was a problem hiding this comment.
I trust your judgement since you certainly understand the pvtdata code better than I do. I just don't like these less-than-transparent conditions where the behaviour of a function is determined by special values of a field that otherwise plays a completely different role in the system (case in point, the txid's principal meaning is to avoid replay attacks and provide a unique handle to a transaction, and it is here used to determine whether data should be retrieved from storage).
These non-obvious relationships (here between txis(=uniquness) and behaviour of pvt data retrieval) tend to be quickly forgotten and make the code hard to maintain.
That said, if you believe this to be the right approach, I won't stand in the way.
wenjianqiao
force-pushed
the
datatriever_no_txid
branch
from
a3a6bdc
to
48b407a
Compare
wenjianqiao
changed the title
… missing Check if txid is available before retrieving private data from the transient store Signed-off-by: Wenjian Qiao <wenjianq@gmail.com>
wenjianqiao
force-pushed
the
datatriever_no_txid
branch
from
48b407a
to
d907722
Compare
|
@Mergifyio backport release-2.3 |
|
@Mergifyio backport release-2.2 |
|
Command
|
|
Command
|
|
Command
|
|
Command
|
…when txid is missing (bp hyperledger#2183) Check if txid is available before retrieving private data from the transient store Signed-off-by: Wenjian Qiao <wenjianq@gmail.com>
Signed-off-by: Wenjian Qiao wenjianq@gmail.com
Type of change
Description
Skip calling fromTransientStore when txid is empty. This could happen for a reconciliation request.
Related issues
https://jira.hyperledger.org/browse/FAB-17039