We're updating the issue view to help you get more done. 

Design: Sort out how to read the tPV and mPV in different contexts

Description

From Slack:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 (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] wo wo 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 I hope 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] oh yeaaaah reminds me 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
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 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] yes i agree 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] true there’s also a `VirtualCordapp` it’s all very confusing an abstraction that’s causing more pain then benefits

Tldr: It's complicated

CVSS Vector

None

Status

Assignee

Tudor Malene

Reporter

Tudor Malene

Labels

CVSS Score

None

Feature Team

Corda Core

Target Version/s

None

Ported to...

None

Priority

Medium