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.

Sign up for GitHub

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

Jump to bottom

Check byKey in transaction validation #9641

Merged
merged 2 commits into from
May 11, 2021
Merged

Check byKey in transaction validation #9641

merged 2 commits into from
May 11, 2021

Conversation

cocreature
Copy link
Contributor

No tests because validation has no tests in general :(

part of #7622

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.

@cocreature
Check byKey in transaction validation
65a65f8 
No tests because validation has no tests in general :(

part of #7622

changelog_begin
changelog_end
remyhaemmerle-da
remyhaemmerle-da approved these changes May 11, 2021
View reviewed changes
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. Thanks.

@remyhaemmerle-da remyhaemmerle-da mentioned this pull request May 11, 2021
Closed
@cocreature
Fix preprocessing of transaction root nodes
955b506 
changelog_begin
changelog_end
cocreature
cocreature commented May 11, 2021
View reviewed changes
...e/src/main/scala/com/digitalasset/daml/lf/engine/preprocessing/TransactionPreprocessor.scala
)
import scala.Ordering.Implicits.infixOrderingOps
val (cmd, newCids) = exe.key match {
case Some(key) if exe.byKey && exe.version >= TransactionVersion.minByKey =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@remyhaemmerle-da can you please check if seems sensible? This was (understandably) necessary to fix the conformance tests.

Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From a general point of view, we have to change this function.
However :

  1. I would prefer we do check for version, i.e. the transaction coder ensures already that exe.byKey is defined only if exe.version >= TransactionVersion.minByKey
Suggested change
case Some(key) if exe.byKey && exe.version >= TransactionVersion.minByKey =>
case Some(key) if exe.byKey =>
  1. We have to do the same thing for Fetch node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer we do check for version, i.e. the transaction coder ensures already that exe.byKey is defined only if exe.version >= TransactionVersion.minByKey

That seems fragile. Now it matters if I first roundtrip via serialization & deserialization or whether I get a transaction directly out of the engine and pass it here. I don’t really see a usecase for the second case but I still feel slightly uneasy about breaking it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The validation was original designed to work only on deserialized transaction. I think we should continue to assume this as:

  • it simplify reasoning about validation as transaction are somehow normalized by the serialization/deserialization process.
  • I do not see use case were we may want to validate non serialized transaction
  • The way we are distributing the versioning logic through the code seems to me very fragile, I really want to limit it to some very limited places. You introduce in this PR tow new places.
  • why changing something that has proven to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough I think my real complaint is that we use the same type for things that have been normalized via serialization & deserialization and things that come directly out of speedy. Yes that might work now (although we also had bugs caused by that iirc) but that’s super fragile under refactoring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be fine with writing a transaction (version aware) normalize equivalent to the serialization-deserialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn’t really solve the issue unless you run that in finish in speedy which probably doesn’t work (e.g., canton needs byKey flags for older versions as well).

What worries me is the mostly implicit assumptions about where we have already normalized and where we haven’t and thereby which checks we can skip and which we cannot skip. The only way to make that reasonably safe that I can see is to have separate types for transactions & nodes that have been normalized and those that come out of speedy. Otherwise this is going to blow up sooner or later.

I agree randomly throwing version checks around like I do here isn’t solving anything. That’s shotgun validation.

On the other hand, the alternative seems to be to randomly throw around normalization like we already had to do for type erasure in a few places which just seems like a different incarnation of the same antipattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was indeed thinking to apply it inside the finish. For canton I see several solution:

  • We add a config flag to normalize or not the byKey flag
  • We include the byKey flag info in the Transaction.MetaData
  • We force Canton to use only LF 1.14 (wish could be somehow possible as there is no Canton production).

@mergify mergify bot merged commit c757b61 into main May 11, 2021
@mergify mergify bot deleted the bykey-validation branch May 11, 2021 16:22
azure-pipelines bot pushed a commit that referenced this pull request May 12, 2021
release 1.13.0-snapshot.20210511.6892.0.ca9e89b3
8eed311 
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.

@victormueller-da is in charge of this release.

Commit log:
```
ca9e89b check whether collection.compat is unused when compiling for Scala 2.12 (#9604)
4a46097 Block concurrent SDK installations. (#9640)
c757b61 Check byKey in transaction validation (#9641)
4f0ceff Fix #7170 (#9618)
d310668 Add a REPL for each Scala target, for debugging. (#9643)
22b36b0 Include byKey in LF transaction spec (#9642)
ca027e3 Add Transaction.contractKeyInputs to inputs of a transaction (#9631)
c3d79d4 Allow OverloadedLists in data-dependencies (#9636)
f8b35fe Cleans stateUpdates of ReadWriteServiceBridge (#9630)
3750b16 Serializalize byKey flag in Daml-LF transactions (#9632)
3b59f8c ledger-api-bench-tool - prototype [DPP-370] (#9606)
f7b33d9 Authorization for daml script exports (#9629)
0d1f3db LF: refactor builtin exceptions in Speedy (#9605)
d6d01b0 Swap order SEScopeExercise and SBUBeginExercise (#9621)
6c919b3 sandbox-on-x: Support `beginAfter` in state updates. (#9624)
26a8011 TLS for Daml Script exports (#9626)
9242540 Make error throw a GeneralError. (#9613)
bdcf0ec update NOTICES file (#9623)
4c1fbeb Add duplicate contract key checks to Speedy (#9607)
871279f LF: extends LF command with Fetch and Lookup  (#9587)
0acc4f1 Patch old Bazel derivation (#9622)
b082274 Address vulnerabilities in Navigator (#9617)
560653a Unwind partial transaction on mustFail (#9608)
f51145c Support mapping from old to new party ids in daml exports (#9591)
89e46bf Address security vulnerabilities in //compiler/daml-extension (#9615)
fd62671 Introduce SINCE-LF-FEATURE in integration tests. (#9616)
48939b5 Address vulnerabilities in //language-support/ts/packages (#9614)
cf59246 Add support for daml exceptions for append-only indexer (#9609)
ab29f7c Move activeness check of globalKeyInputs into archive (#9610)
5c28de3 Upgrade grunt to address cve (#9611)
22ba5fd Remove builtin exception types from protobuf and ASTs. (#9595)
b09a95f Adapt indexer to empty divulgences (#9598)
2fc7489 Filter divulgence to an empty set of parties (#9600)
ad45213 participant-integration-api: Avoid the serial EC in integration testing. (#9603)
f742a43 Dpp 336 sandbox classic on append only (#9561)
e68bc0d Mark reset service tests as flaky (#9602)
2176173 Remove 1.dev-only things from LF 1.13 protofile. (#9599)
03034ec DPP-359 Add indexer flow level benchmark (#9509)
5aa5401 Drop todo for conversion of exception primitives (#9597)
968b5d8 KVL-921 Expose opentelemetry Context from Telemetry and TelemetryContext (#9573)
112c387 Refactor out setGlobalLogLevel into ContextualLogging (#9592)
e584fec Drop com.daml.lf.engine.Event.collectEvents (#9596)
80b07da Only archive a key if it was brought into scope before (#9546)
7ea9340 Support old bash in daml-sdk-head (#9593)
45fbdef Handle rollback nodes in protoNodeInfo (#9589)
42d189d Support exceptions in Daml-LF encoder (#9590)
9c3913a LF: SBUKeyBuiltin clean up. (#9572)
5128206 Ledger API Server: Named threadpools (#9588)
26a53d8 Add logging command line option to ledger http service (#9581)
409833f Address rollback todo in ancient migration (#9583)
a27b2c5 Address more exception todos (#9582)
2040556 Support rollback nodes in replay adapter (#9584)
192a77a update compat versions for 1.13.0-snapshot.20210504.6833.0.9ae787d0 (#9575)
81d508b LF: make SBuiltins pure (#9580)
de80a6d Drop ValueBuiltinException (#9576)
1c456be Release yet another 1.13 snapshot (#9574)
8009891 Disable migration tests on macos on PRs (#9571)
2e93de7 Fix recording exceptions in Spans, add unit tests [KVL-874] (#9544)
121ded3 Accept a comma-separated list of parties for daml ledger export (#9568)
```
Changelog:
```
- [Daml Assistant] The assistant will now avoid
  installing SDK versions concurrently, waiting
  for the previous installation to finish before
  starting the next installation (if still
  necessary). This fixes a bug where the SDK
  would become corrupted because two
  installations were started concurrently.
http-json:
- add contextual id as logging context to distinguish different application runs in logs
- add request id as logging context to distinguish different http requests within an application run
- add for non-static endpoints trace logs which show how long processing it took in ns
- [Integration Kit] - Created the ledger-api-bench-tool prototype for benchmarking ledger transaction streaming capabilities
* [Daml export] Users can now define a mapping from parties in the
  original ledger state to parties to be used when recontructing the
  ledger state. The ``parties`` component of the argument to the
  generated export script now takes a mapping from old party name to new
  party name.
- [Ledger HTTP Json Service] Logging now also tells service name if log level was changed.
- [Ledger HTTP Json service] Logging can now be configured via the `--log-level` cli argument. Valid values are `error`, `warn`, `info` (default), `debug`, `trace`
```

CHANGELOG_BEGIN
CHANGELOG_END
realvictorprm added a commit that referenced this pull request May 12, 2021
@azure-pipelines @realvictorprm
release 1.14.0-snapshot.20210511.6892.0.ca9e89b3 (#9647)
4620851 
* release 1.13.0-snapshot.20210511.6892.0.ca9e89b3

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.

@victormueller-da is in charge of this release.

Commit log:
```
ca9e89b check whether collection.compat is unused when compiling for Scala 2.12 (#9604)
4a46097 Block concurrent SDK installations. (#9640)
c757b61 Check byKey in transaction validation (#9641)
4f0ceff Fix #7170 (#9618)
d310668 Add a REPL for each Scala target, for debugging. (#9643)
22b36b0 Include byKey in LF transaction spec (#9642)
ca027e3 Add Transaction.contractKeyInputs to inputs of a transaction (#9631)
c3d79d4 Allow OverloadedLists in data-dependencies (#9636)
f8b35fe Cleans stateUpdates of ReadWriteServiceBridge (#9630)
3750b16 Serializalize byKey flag in Daml-LF transactions (#9632)
3b59f8c ledger-api-bench-tool - prototype [DPP-370] (#9606)
f7b33d9 Authorization for daml script exports (#9629)
0d1f3db LF: refactor builtin exceptions in Speedy (#9605)
d6d01b0 Swap order SEScopeExercise and SBUBeginExercise (#9621)
6c919b3 sandbox-on-x: Support `beginAfter` in state updates. (#9624)
26a8011 TLS for Daml Script exports (#9626)
9242540 Make error throw a GeneralError. (#9613)
bdcf0ec update NOTICES file (#9623)
4c1fbeb Add duplicate contract key checks to Speedy (#9607)
871279f LF: extends LF command with Fetch and Lookup  (#9587)
0acc4f1 Patch old Bazel derivation (#9622)
b082274 Address vulnerabilities in Navigator (#9617)
560653a Unwind partial transaction on mustFail (#9608)
f51145c Support mapping from old to new party ids in daml exports (#9591)
89e46bf Address security vulnerabilities in //compiler/daml-extension (#9615)
fd62671 Introduce SINCE-LF-FEATURE in integration tests. (#9616)
48939b5 Address vulnerabilities in //language-support/ts/packages (#9614)
cf59246 Add support for daml exceptions for append-only indexer (#9609)
ab29f7c Move activeness check of globalKeyInputs into archive (#9610)
5c28de3 Upgrade grunt to address cve (#9611)
22ba5fd Remove builtin exception types from protobuf and ASTs. (#9595)
b09a95f Adapt indexer to empty divulgences (#9598)
2fc7489 Filter divulgence to an empty set of parties (#9600)
ad45213 participant-integration-api: Avoid the serial EC in integration testing. (#9603)
f742a43 Dpp 336 sandbox classic on append only (#9561)
e68bc0d Mark reset service tests as flaky (#9602)
2176173 Remove 1.dev-only things from LF 1.13 protofile. (#9599)
03034ec DPP-359 Add indexer flow level benchmark (#9509)
5aa5401 Drop todo for conversion of exception primitives (#9597)
968b5d8 KVL-921 Expose opentelemetry Context from Telemetry and TelemetryContext (#9573)
112c387 Refactor out setGlobalLogLevel into ContextualLogging (#9592)
e584fec Drop com.daml.lf.engine.Event.collectEvents (#9596)
80b07da Only archive a key if it was brought into scope before (#9546)
7ea9340 Support old bash in daml-sdk-head (#9593)
45fbdef Handle rollback nodes in protoNodeInfo (#9589)
42d189d Support exceptions in Daml-LF encoder (#9590)
9c3913a LF: SBUKeyBuiltin clean up. (#9572)
5128206 Ledger API Server: Named threadpools (#9588)
26a53d8 Add logging command line option to ledger http service (#9581)
409833f Address rollback todo in ancient migration (#9583)
a27b2c5 Address more exception todos (#9582)
2040556 Support rollback nodes in replay adapter (#9584)
192a77a update compat versions for 1.13.0-snapshot.20210504.6833.0.9ae787d0 (#9575)
81d508b LF: make SBuiltins pure (#9580)
de80a6d Drop ValueBuiltinException (#9576)
1c456be Release yet another 1.13 snapshot (#9574)
8009891 Disable migration tests on macos on PRs (#9571)
2e93de7 Fix recording exceptions in Spans, add unit tests [KVL-874] (#9544)
121ded3 Accept a comma-separated list of parties for daml ledger export (#9568)
```
Changelog:
```
- [Daml Assistant] The assistant will now avoid
  installing SDK versions concurrently, waiting
  for the previous installation to finish before
  starting the next installation (if still
  necessary). This fixes a bug where the SDK
  would become corrupted because two
  installations were started concurrently.
http-json:
- add contextual id as logging context to distinguish different application runs in logs
- add request id as logging context to distinguish different http requests within an application run
- add for non-static endpoints trace logs which show how long processing it took in ns
- [Integration Kit] - Created the ledger-api-bench-tool prototype for benchmarking ledger transaction streaming capabilities
* [Daml export] Users can now define a mapping from parties in the
  original ledger state to parties to be used when recontructing the
  ledger state. The ``parties`` component of the argument to the
  generated export script now takes a mapping from old party name to new
  party name.
- [Ledger HTTP Json Service] Logging now also tells service name if log level was changed.
- [Ledger HTTP Json service] Logging can now be configured via the `--log-level` cli argument. Valid values are `error`, `warn`, `info` (default), `debug`, `trace`
```

CHANGELOG_BEGIN
CHANGELOG_END

* Release snapshot 1.14.0
changelog_begin
changelog_end

Co-authored-by: Azure Pipelines DAML Build <support@digitalasset.com>
Co-authored-by: victor.mueller@digitalasset.com <mueller.vpr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remyhaemmerle-da remyhaemmerle-da remyhaemmerle-da approved these changes

@nickchapman-da nickchapman-da Awaiting requested review from nickchapman-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cocreature @remyhaemmerle-da