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

Support rollback nodes in BlindingInfo #9445

Merged
merged 1 commit into from
Apr 20, 2021
Merged

Conversation

cocreature
Copy link
Contributor

changelog_begin
changelog_end

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

changelog_begin
changelog_end
@@ -13,6 +13,9 @@ import com.daml.lf.value.Value.ContractId
* "Disclosure" tells us which transaction nodes to communicate to which
* parties.
* See https://docs.daml.com/concepts/ledger-model/ledger-privacy.html
* Note that rollback nodes are a bit special here: Even if the node
* itself is not disclosed, a party will be disclosed if a node was below a
* rollback node. That information is not included in blinding info at this point.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might want to revisit this later but this is enough for the scenario service and sandbox classic and I’d rather start simple and add something later if we do end up needing it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, but I would have added a // TODO https://github.com/digital-asset/daml/issues/8020 with this comment inside the code.

@cocreature cocreature requested a review from oggy- April 19, 2021 15:34
Copy link
Contributor

@nickchapman-da nickchapman-da left a comment

Choose a reason for hiding this comment

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

LGTM 👍

One small thing. I see that following the GenActionNode / RollbackNode split, we have removed NodeInfo from rollback nodes. So consequently, unlike other node types, there is no informeesOfNode (or requiredAuthorizers) for a rollback node. Is this correct?

@cocreature
Copy link
Contributor Author

One small thing. I see that following the GenActionNode / RollbackNode split, we have removed NodeInfo from rollback nodes. So consequently, unlike other node types, there is no informeesOfNode (or requiredAuthorizers) for a rollback node. Is this correct?

Exactly, only actions have informees. Rollback nodes are not actions. However, we can still disclose a rollback node to the informees of the parent exercise.

@cocreature cocreature merged commit cf2be78 into main Apr 20, 2021
@cocreature cocreature deleted the blinding-info-rollback branch April 20, 2021 18:29
azure-pipelines bot pushed a commit that referenced this pull request Apr 21, 2021
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@remyhaemmerle-da is in charge of this release.

Commit log:
```
cf2be78 Support rollback nodes in BlindingInfo (#9445)
adde90b KV: do not use "Effects" part of InputsAndEffects (#9433)
75fa86a Additional metrics for mutable contract state cache (#9444)
d8c34a4 Rename normalization-related params in the integrity checker config (#9455)
e335244 Hash submitters in the deduplication key [DPP-347] (#9417)
43ffa42 Test for duplicate contracts when querying on behalf of multiple parties (#9443)
7644844 Document daml ledger export script (#9446)
3193027 DPP-334 Manually partition the event table (#9394)
14661a8 Clearer variable name for subtype evidence (#9442)
40c85d8 Release v1.13.0-snapshot.20210419.6730.0.8c3a8c04 (#9439)
1cb907c KV: do not use "Inputs" part of InputsAndEffects (#9429)
```
Changelog:
```
- [Integration Kit] - deduplication key will contain hashed submitters instead of concatenated submitters
* [Daml export] Refer to the "Ledger Export" chapter under the "Early
  Access Features" for a description of the new Daml ledger export
  command. This is an early access feature.
```

CHANGELOG_BEGIN
CHANGELOG_END
Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +70 to +71
case _: Node.NodeCreate[ContractId] => state
case _: Node.NodeLookupByKey[ContractId] => state
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
case _: Node.NodeCreate[ContractId] => state
case _: Node.NodeLookupByKey[ContractId] => state
case _: Node.NodeCreate[ContractId] | _: Node.NodeLookupByKey[ContractId] => state

Comment on lines +97 to 103
rollback.children.foldLeft(state) { (s, childNodeId) =>
processNode(
s,
witnesses,
parentExerciseWitnesses,
childNodeId,
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
rollback.children.foldLeft(state) { (s, childNodeId) =>
processNode(
s,
witnesses,
parentExerciseWitnesses,
childNodeId,
)
}
rollback.children.foldLeft(state) ((s, childNodeId) =>
processNode(
s,
parentExerciseWitnesses,
childNodeId,
)
)

or

Suggested change
rollback.children.foldLeft(state) { (s, childNodeId) =>
processNode(
s,
witnesses,
parentExerciseWitnesses,
childNodeId,
)
}
rollback.children.foldLeft(state)(processNode(_, parentExerciseWitnesses, _))

@@ -13,6 +13,9 @@ import com.daml.lf.value.Value.ContractId
* "Disclosure" tells us which transaction nodes to communicate to which
* parties.
* See https://docs.daml.com/concepts/ledger-model/ledger-privacy.html
* Note that rollback nodes are a bit special here: Even if the node
* itself is not disclosed, a party will be disclosed if a node was below a
* rollback node. That information is not included in blinding info at this point.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, but I would have added a // TODO https://github.com/digital-asset/daml/issues/8020 with this comment inside the code.

@remyhaemmerle-da
Copy link
Collaborator

part of #8020

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.

3 participants