Optimising map_new_from_slices for optional / void values

[leighmcculloch]

I propose that in a future version of the protocol the map_new_from_slices and map_unpack_to_slice function be modified so that when writing or reading the map of values:

• When writing a Soroban 'void' value for a key-value pair, that the key-value pair be omitted from being written / stored.
• When reading a missing key-value pair, that the value for that key-value pair be assumed to be the 'void' value and returned as such.

Some context:

• Option<T> is a Rust type that in Soroban is converted into the following values:

  • When the option has the Some value containing a value of type T, the option is converted into a Soroban value that matches the T type.
  • When the option has the None value that does not contain a value of T, the option is converted into a Soroban 'void' value.

• Whenever any contracttype struct is converted into an ScVal, it is converted into a Map, and a host function specifically designed for optimised passing of maps back and forth between the guest and the host is used. That host function is map_new_from_slices.

I am in the process of implementing the contractevent macro, that will provide a way to define an event using a struct. In conversation for CAP-67 an idea was suggested (by @dmkozh I think?) that any SDK functionality that's built to create events where you can specify optional data fields in a map, should omit key-value pairs where the optional data field has no value. It would be convenient, intuitive, and a reasonable optimisation.

Why:

What I've found whilst experimenting with a couple different implementations of a contractevent macro is that we will end up using the same map_new_from_slices that contracttype uses for converting structs to maps. So if we want that behaviour of omitting fields that have a none optional, we either need to change the existing function, or add a new function that behaves similarily except for the omission part.

I think the change should be made to map_new_from_slices rather than introduce a new function, because consistency between how data is omitted for contracttype and contractevent is important. If we were to make contractevent omit fields while contracttype did not, there would be visible differences with how contract events get written if they contain a single-value that just happens to be a map, vs the event itself be a multi-value map. Whilst the network doesn't distinguish between those two event structures, contract code will.

I think the optimisation of omitting key-value pairs for optionals without a value is reasonable to make to storage too, not just events. So making this change in such a way that the omission occurrs also for contracttypes is a net benefit.

Additional benefits:

An additional benefit of this change is that it will become easier to migrate data in contracts. Today once a contracttype struct is stored, it cannot gain any new fields, even if they are optional, because reading out any old value will error on the missing new field on old storage. This change would allow new fields to be missing on old stored values, as long as the type of the new field is Option<T>.

Backwards compatibility:

And as long as the read handles the case where the key-value pair is missing, it is largely backwards compatible with existing usage.

This is however a change in behaviour that would be observable in some edge cases. For example, if you rely on contract storage erroring when reading out a type that is missing a value that maps to an option, that would no longer be the case. But that case seems quite fringe.

cc @stellar/contract-committers

[dmkozh]

I like the suggestion approach in general. The only thing I'm not sure about is whether we should be changing the semantics of the existing host function. This seems a bit risky to me, even though I agree that it seems hard to come up with the exact scenario where this could introduce a vulnerability or a logic bug. Still, the fact that we can't easily come up with a scenario doesn't mean that there will be no such scenario.

If we were to make contractevent omit fields while contracttype did not,

This is not an argument for modifying the existing function, is it? Nothing prevents us from introducing a new host function in p24 and then using it in SDK v24 macro implementations. This way we guarantee that the old contracts behave as expected, and the new contracts can make an educated decision about using the new semantics when performing an upgrade.

[leighmcculloch]

I'm not sure about is whether we should be changing the semantics of the existing host function

+1 A _v2 suffix would suffice I think on a new host function with the new behaviour.

In practice some unpredictable risk remains. The moment a dev updates their soroban-sdk they'll start using that new function, so a developer isn't really in control of when the change occurs, and it's largely invisible to them, meaning it is still possible someone updates their sdk and unexpectedly breaks a downstream system because of an expectation on the field being written as void rather than omitted. But at least that breakage is controlled at the contract update point, with a new wasm, so it's a bit better in that the change can be explained well in the context of different wasm/stdlib.

[dmkozh]

Yes, I realize that it's not perfect, but at least we can inform the developers via release notes, there is a chance that unit or integration tests will fail, and developers would probably test the upgrade on testnet first. So there is quite a few places where a change can be observed. If we change the protocol retroactively, the contract behavior may change immediately and in some cases developers may not even have a possibility to upgrade it.
I think we are on the same page here, and it seems like a pretty simple change to make in protocol 24.

[jayz22]

Would it be possible to solve this at the sdk level? I.e. in the macro generation code check for Void type and skipping entries. If so it would be easier and more future proof than adding a new host function.

[leighmcculloch]

Worth a try to see how it looks:

• Support optional fields in contracttype and contractevent by omitting None values from Maps rs-soroban-sdk#1711

[leighmcculloch]

I implemented a spike of this:

• Omit None-valued Option fields from maps rs-soroban-sdk#1712

The contracttype and contractevent generated code cannot use map_new_from_slices and map_unpack_to_slice at all for struct types that contain optional fields. That means replacing 1 host function call with 1-2 host function calls per field in the map. For an event with 4 fields, that is 4-8 host function calls vs 1.

Evaluating the impact with the test_events test contract that lives in the rs-soroban-sdk repo:

Contract size is 0.2KB larger.

Execution cost difference:

Before:

┌───────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Fee Proposed: 14456                                                                                   │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Resource Fee: 14356                                                      │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Non-Refundable: 3373         │ Refundable: 10983                         │
│                            │                              │                                           │
│                            │ cpu instructions             │ return value                              │
│                            │ storage read/write           │ storage rent                              │
│                            │ tx size                      │ events                                    │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│                                     👆 Proposed Fee  👇 Final Fee                                     │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Non-Refundable Charged: 3373 │ Refundable Charged: 1133 │ Refunded: 9850 │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Resource Fee Charged: 4506                              │ Refunded: 9850 │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Fee Charged: 4606                                                                    │ Refunded: 9850 │
└───────────────────────────────────────────────────────────────────────────────────────────────────────┘

After:

┌───────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Fee Proposed: 14330                                                                                   │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Resource Fee: 14230                                                      │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Non-Refundable: 3381         │ Refundable: 10849                         │
│                            │                              │                                           │
│                            │ cpu instructions             │ return value                              │
│                            │ storage read/write           │ storage rent                              │
│                            │ tx size                      │ events                                    │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│                                     👆 Proposed Fee  👇 Final Fee                                     │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Non-Refundable Charged: 3381 │ Refundable Charged: 1016 │ Refunded: 9833 │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Resource Fee Charged: 4397                              │ Refunded: 9833 │
├───────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Fee Charged: 4497                                                                    │ Refunded: 9833 │
└───────────────────────────────────────────────────────────────────────────────────────────────────────┘

Evaluating the impact with the test_udt test contract that lives in the rs-soroban-sdk repo:

Contract size is 0.2KB larger.

Execution cost difference:

Before:

┌─────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Fee Proposed: 14054                                                                                 │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Resource Fee: 13954                                                    │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Non-Refundable: 4208         │ Refundable: 9746                        │
│                            │                              │                                         │
│                            │ cpu instructions             │ return value                            │
│                            │ storage read/write           │ storage rent                            │
│                            │ tx size                      │ events                                  │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│                                    👆 Proposed Fee  👇 Final Fee                                    │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Non-Refundable Charged: 4208 │ Refundable Charged: 59 │ Refunded: 9687 │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Resource Fee Charged: 4267                            │ Refunded: 9687 │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Fee Charged: 4367                                                                  │ Refunded: 9687 │
└─────────────────────────────────────────────────────────────────────────────────────────────────────┘

After:

┌─────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Fee Proposed: 14072                                                                                 │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Resource Fee: 13972                                                    │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee: 100         │ Non-Refundable: 4226         │ Refundable: 9746                        │
│                            │                              │                                         │
│                            │ cpu instructions             │ return value                            │
│                            │ storage read/write           │ storage rent                            │
│                            │ tx size                      │ events                                  │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│                                    👆 Proposed Fee  👇 Final Fee                                    │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Non-Refundable Charged: 4226 │ Refundable Charged: 59 │ Refunded: 9687 │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Inclusion Fee Charged: 100 │ Resource Fee Charged: 4285                            │ Refunded: 9687 │
├─────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Fee Charged: 4385                                                                  │ Refunded: 9687 │
└─────────────────────────────────────────────────────────────────────────────────────────────────────┘

[leighmcculloch]

I have updated the description at the top to note that two host functions need modifying or two new host functions need creating. The proposal originally only mentioned map_new_from_slices , but map_unpack_to_slice also needs modifying because it is the function that unpacks a Map into a slice and it would need to be updated to unpack Val::VOID into the slice positions that correlate to the fields that are missing.

[leighmcculloch]

@tomerweller asked what the cost-benefit of this change would be.

For token events, the main use case is the to_muxed_id map field could be omitted. That field takes up 24 bytes in events.

$ echo '{"key":{"symbol":"to_muxed_id"},"val":"void"}' | stellar xdr encode --type ScMapEntry --output single | xxd
00000000: 0000 000f 0000 000b 746f 5f6d 7578 6564  ........to_muxed
00000010: 5f69 6400 0000 0001                      _id.....

For all contracts, any optional fields will reduce storage entries by a similar amount.

@tomerweller I think the benefit is there, more so for migration of storage data than for the original use case where the idea was birthed (event publishing).

Opening up the ability to do incremental migration easier using optional fields is a meaningful improvement.

Today to do incremental migration when adding new fields, two types have to be defined and there must be a way to know that you need to read one vs the other, which means storing parallel to every type a version marker of sorts. This is a robust strategy for doing migration. But less natural and obvious than simply adding an optional field on a type.

[leighmcculloch]

I am closing this proposal, in favour of a new proposal that I'd like to be considered instead:

• Improving Storage Migration with Changes to `map_new_from_slices`, `map_unpack_to_slice`, and `contracttype` #1877