(Yet another) Issue with cordapp dependencies + `targetPlatformVersion`/`minPlatformVersion`:
Let’s say someone writes some corda toolkit and releases it. (`FooToolkit`)
They develop it against Corda 6 so `tPv=6` and `mPv=5` (they use a v5 api).
Then someone develops a `BarContract` that uses the `FooToolkit`.
They will need to set the `mPv = max( version_apis_they_use_directly, version_apis_called_indirectly_by_all_dependencies)`.
Even if the toolkit has `mPv=5`, if `BarContract` uses only a small subset that does not call any v5 apis then there is no need to set `mPv=5`.
It’s more tricky when it comes to the `tPv`:
If `BarContract` fatJars the dependency then the `tPv` metadata from the `FooToolkit` is lost. (as it currently stands)
If the final JAR has a `tPv=7` then the API calls made from the fatJarred `FooToolkit` will return v7 compatible results which will be incorrect (as it would have expected v6 results).
If they don’t fatJar, we still have to adjust how we read the tPv from the call stack. (Currently it’s the tpv of the contract jar of the contract you are currently verifying)
I think it must be: “use the `tpv` of the exact jar that makes the corda api call”.
A less ideal alternative is when bundling the `BarContract` fatJar to set `tpv= min(all_dependencies, current_corda)`.
This is a recursive problem as the sdk can use a similar mpv/tpv mechanism for its own api.
mike [1:11 PM]
yes, the reason we do a stack walk when deciding tpv is to catch the case of one module calling into another
of course, you cannot get every situation right. these things are in the end only a set of kludges
we should be careful not to get too trigger-happy with tPV i suppose - it should be restricted for cases where we're sure we're likely to be breaking apps if we don't do it
we likely should also avoid artificially feature-tying. i think we did that in v4 (you only get nice goodies if you target v4) and it can likely create nasty triangle-of-death scenarios. we should use it only for backwards compatibility and not to encourage keeping up to date
whilst in theory this stuff is all recursive, only quite advanced platform builders use/care about this sort of backwards compatibility. it matters if you plan to have hundreds or thousands or millions of apps
for a customer like b3i, how many apps will realistically be built on top of fluidity? 10?
recursively creating their own notion of platform versions, target version etc is likely not worth the cost for them
i could be wrong about this. maybe we'll see demand from them to have access to this infrastructure themselves. they're pretty advanced.
tudor [1:15 PM]
actually we don’t walk the stack for contracts
mike [1:15 PM]
roger's team is also likely to ask for this
ah ok. maybe we should.
tudor [1:16 PM]
we consider the tpv to be the one of the contract jar
I don’t think walking the stack is the way to go though. It needs to be the tpv of the jar making the api call
mike [1:17 PM]
yes, and to know what jar is calling you, you need to walk the stack (consider the tpv check might be in a function called by another platform function)
tudor [1:17 PM]
ah yes. true
mike [1:22 PM]
sharpening the tpv system is another good reason to get rid of fat-jars. i'm very worried the tokens team are going to find someone launched with the tokens SDK as it exists now (separate jar) and we will have a security problem on our hands that they will blame us for, because they were only following instructions
tudor [1:23 PM]
I’ll raise a ticket to modify how we calculate the tpv now for contract logic
mike [1:26 PM]
can you dig in to why it was implemented that way originally?
tudor [1:26 PM]
I was looking at it now
mike [1:26 PM]
the original proposed design for tpv checking was pretty explicit about stack walking. if it's done differently whoever wrote it presumably had a reason
Stefano Franz [1:26 PM]
tokens is explicitly _not_ a fat jar
tudor [1:27 PM]
only 2 wos? :slightly_smiling_face:
Stefano Franz [1:27 PM]
that is why we encounter the attachment issue
ran out of time
mike [1:27 PM]
yes i know, and that may have to change. you don't want to create a security issue for our users, right?
we are faced with no really great options in this case, i fear
just a search for the least-worst outcome
tudor [1:34 PM]
we have 2 apis. First we use for flows. It’s a static: `CordappResolver.currentCordapp` that walks the stack until it finds a class that is in one of the installed cordapps, and returns it
for contracts, we just have logic for @belongsToContract
which looks at the tpv of the contracts jar
which sounds correct
Also used for the no-overlap rule to exclude jolokia (edited)
Actually the tpv check for the no-overlap rule is for each jar in the classloader
Stefano Franz [1:41 PM]
I don't think any of this is true
I literally put together a PoC with jar based dependencies
any given cordapp can have a well known set of dependencies
bashed on the sha256 at build time
mike [1:43 PM]
the PoC is a platform upgrade (i.e. needs a new mPV) and isn't merged yet. but yeah i think tudor has been concluding that it's in the right general direction
Stefano Franz [1:43 PM]
it doesn't need a new mPV at the network level though
it does need a platform version upgrade for cordapps targetting this functionality
tudor [1:43 PM]
That’s the right direction *only* if we use a classloader per cordapp
Stefano Franz [1:44 PM]
I was meant to reply to that email
mike [1:45 PM]
@tudor even if we (currently) don't, it takes us in the right direction doesn't it? albeit, rather than a quick PoC PR it'd be good if we had a design doc that considers some of the hierarchy issues we discussed at b3i
i suspect there's very little gaps in our thinking. stefano's PoC can be finished off, upgraded, documented, toolified, put into the platform. imho we _should_ make it generalised, i.e. not just roll it into core, it should be (you'll love this stefano) its own module
tudor [11:04 AM]
@mike, I realised that `CordappResolver.currentTargetVersion` is not totally correct either.
In the cordapps folder there can be multiple JARs, but we only consider to be “Cordapps” the ones that have Corda artifacts in them.
When calling `CordappResolver.currentCordapp` or `CordappResolver.currentTargetVersion` it is only stack frames that belong to “Cordapps” that are used.
The problem is when there’s a utilities library in the `cordapps` folder that happens to call Corda APIs, as it will be ignored by the `CordappResolver`.
It’s not a biggie, but I think it showcases a problem with the: `net.corda.core.cordapp.Cordapp` abstraction we’re using.
I think the initially assumption was that an entire Cordapp would live in a JAR (flows, contracts, dependencies).
(Afterwards it appears we added some versioning info to that, and created some Contracts/Workflow specific classes.)
This doesn’t really match how things are in the wild.
Other signs that something is wrong are:
- all the pain and confusion during tests with package scanning and `findCordapp`.
- Also the Classgraph scanning issues (if you’re scanning individual jars and are missing intermediary classes, then it won’t detect that you’re implementing some interface)
I wonder if this is the right abstraction and if it’s time to revisit it.
mike [11:07 AM]
stack walks should simply stop at the first jar that exposes a target version. this sort of versioning logic is itself something we could extract into a module and make re-usable by other projects outside of corda ... it'd only require a bit of additional metadata to be addable to manifests saying _which_ platform you're targeting
or maybe an extended format, e.g. `Target-Version: 6:corda, 11:java` for example :slightly_smiling_face:
tudor [11:12 AM]
yes, that would work for stack walking
should we not also rethink `net.corda.core.cordapp.Cordapp` with the benefit of the hindsight we have now?
mike [11:14 AM]
maybe, but that's public api
tudor [11:15 AM]
I was thinking to create something parallel and use that internally, and keep the Cordapp api as is
( maybe bridged on demand from the new stuff)
mike [11:15 AM]
i'd hope with a more jar-centric view, the standard java apis would be most of it
tudor [11:16 AM]
there’s also a `VirtualCordapp`
it’s all very confusing
an abstraction that’s causing more pain then benefits
Tldr: It's complicated