Config Setting upgrades by sisuresh · Pull Request #3674 · stellar/stellar-core

sisuresh · 2023-03-09T23:25:03Z

Description

Add a mechanism to propose upgrades that are stored in ContractData ledger entries. This PR is built on top of #3522, and starts with the "ConfigSettingEntry upgrades" commit.

Related xdr change - stellar/stellar-xdr#75.

xdr has not been merged in yet so this PR will fail until that happens.

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

src/bucket/test/BucketListTests.cpp

src/herder/Upgrades.cpp

dmkozh · 2023-03-10T19:06:57Z

src/herder/Upgrades.cpp

I'm not sure if this has the same format as all the other entries (with '='). Also, this will return JSON, right? So the formatting might be a bit weird due to it being multiline. Not sure though where we're using the toString though...

src/herder/Upgrades.cpp

src/herder/test/UpgradesTests.cpp

dmkozh · 2023-03-10T20:07:28Z

src/herder/test/UpgradesTests.cpp

NB: I believe exception is fine here as we shouldn't get to upgrade execution for the invalid/unknown upgrades. But we need to make sure we don't crash during validation itself.

So this is still an open question I have. If a valid configUpgrade is nominated, is it possible so change the underlying ContractData entry that holds the ConfigUpgradeSet before the upgrade is applied? If so, that scenario needs to be handled.

Hmm, yeah, it might change, so we need to verify that hash in the key matches the content hash. That can happen at validation time though, as upgrades happen after all the transactions. At apply time the upgrade set should be guaranteed to be correct and hence it should be ok to throw an exception (as that would be a bug)

The hash is already verified in ConfigUpgradeSetFrame::isValidXDR.

So we should be good, right? Also we load the actual entry just once anyway, so there shouldn't be mismatch between the config upgrade set at validation and application time.

We load the entry at validation and at apply (every time we call makeFromKey). It's possible that the ledger the upgrade should be included in changes the entry, making the upgrade invalid. In that case, an exception would be thrown here -

stellar-core/src/ledger/LedgerManagerImpl.cpp

Line 744 in 53ea43a

throw std::runtime_error(

We need to take this into account and handle the invalid upgrade case instead of crashing.

Updated code to ignore invalid upgrades instead of throwing an exception.

src/herder/Upgrades.cpp

src/herder/test/UpgradesTests.cpp

src/herder/Upgrades.cpp

dmkozh · 2023-03-21T21:44:08Z

src/herder/test/UpgradesTests.cpp

Hmm, yeah, it might change, so we need to verify that hash in the key matches the content hash. That can happen at validation time though, as upgrades happen after all the transactions. At apply time the upgrade set should be guaranteed to be correct and hence it should be ok to throw an exception (as that would be a bug)

dmkozh · 2023-03-21T21:45:15Z

src/herder/Upgrades.cpp

Just wanted to make sure, how the downstream code handles nullptr here? Would it just act as if we didn't have such upgrade armed?

It depends. In the CommandHandler, we actually check after setting to make sure something was set. In applyTo, we throw an exception (this shouldn't happen, but if it does, it'll be caught). In isValidForApply, we return XDR_INVALID. In isValidForNomination, we return false.

src/herder/Upgrades.cpp

src/bucket/test/BucketListTests.cpp

src/herder/Upgrades.cpp

MonsieurNicolas · 2023-03-25T01:31:43Z

src/herder/Upgrades.cpp

+                   hexAbbrev(key.contentHash));
+        return false;
+    }
+    if (std::adjacent_find(


is_sorted above already does not allow for duplicates

That's not true - it 'Checks if the elements in range [first, last) are sorted in non-descending order'

bleh yes! I always get is_sorted wrong

So I guess the suggestion becomes: as we use a custom predicate we should just use >= for adjacent_find which should do what we want (can't use this with is_sorted as >= is not Compare).

src/herder/Upgrades.cpp

src/main/CommandHandler.cpp

src/herder/Upgrades.cpp

MonsieurNicolas · 2023-03-29T04:49:55Z

Change looks good now!

This is to support how the new XDR is generated.

While this can be supported from the ledger standpoint, I think that semantically these shouldn't be removed and hence it's better to introduce some harness to make sure we don't accidentally remove it.

- Add methods to generate unique entries, so that we don't need to exclude `CONFIG_SETTING` entry from the fuzzer due to them having key collisions - Add methods to generate entries of given type and entries with exclusions to be more explicit about including/excluding the entry types where relevant

…of `CONFIG_SETTING` entries.

sisuresh · 2023-03-30T00:24:56Z

I cleaned up history and merged in the xdr change. One thing I'm trying to figure out though - I only updated the C++ and not env, but the xdr mismatch check isn't triggering for me locally.

MonsieurNicolas · 2023-03-30T03:23:01Z

I cleaned up history and merged in the xdr change. One thing I'm trying to figure out though - I only updated the C++ and not env, but the xdr mismatch check isn't triggering for me locally.

@graydon do you think this is related to the fix you made?

sisuresh · 2023-03-30T15:41:34Z

I cleaned up history and merged in the xdr change. One thing I'm trying to figure out though - I only updated the C++ and not env, but the xdr mismatch check isn't triggering for me locally.

@graydon do you think this is related to the fix you made?

Looks like the github action correctly failed though. Weird.

sisuresh · 2023-03-30T16:11:52Z

I cleaned up history and merged in the xdr change. One thing I'm trying to figure out though - I only updated the C++ and not env, but the xdr mismatch check isn't triggering for me locally.

@graydon do you think this is related to the fix you made?

Looks like the github action correctly failed though. Weird.

Ah I see this in my make output. I got a new laptop recently which is why I haven't run into this before. I'll try to get this to fail the build.

../hash-xdrs.sh: line 25: sha256sum: command not found

graydon · 2023-04-06T04:32:53Z

r+ cd3d956

graydon · 2023-04-06T04:33:47Z

(@MonsieurNicolas is on vacation but I believe all his concerns were addressed, I'm going to dismiss his change-request)

on vacation, all changes addressed

sisuresh requested a review from dmkozh March 9, 2023 23:25

dmkozh reviewed Mar 10, 2023

View reviewed changes

sisuresh force-pushed the config_setting_le branch 3 times, most recently from a07289b to b330107 Compare March 20, 2023 18:38

sisuresh commented Mar 20, 2023

View reviewed changes

src/herder/Upgrades.cpp Outdated Show resolved Hide resolved

sisuresh mentioned this pull request Mar 21, 2023

XDR for upgradeable config entries stellar/stellar-xdr#75

Merged

dmkozh reviewed Mar 21, 2023

View reviewed changes

sisuresh force-pushed the config_setting_le branch 2 times, most recently from 78a46c4 to 7e47c86 Compare March 23, 2023 22:28

sisuresh marked this pull request as ready for review March 23, 2023 22:30

sisuresh commented Mar 23, 2023

View reviewed changes

src/herder/Upgrades.cpp Outdated Show resolved Hide resolved

MonsieurNicolas previously requested changes Mar 25, 2023

View reviewed changes

MonsieurNicolas reviewed Mar 25, 2023

View reviewed changes

src/herder/Upgrades.cpp Outdated Show resolved Hide resolved

MonsieurNicolas reviewed Mar 25, 2023

View reviewed changes

src/herder/Upgrades.cpp Show resolved Hide resolved

sisuresh force-pushed the config_setting_le branch from 80b11d3 to 77bcdd3 Compare March 28, 2023 00:45

MonsieurNicolas reviewed Mar 28, 2023

View reviewed changes

src/herder/Upgrades.cpp Outdated Show resolved Hide resolved

sisuresh force-pushed the config_setting_le branch from 14f73ba to ed002c2 Compare March 30, 2023 00:20

dmkozh added 5 commits March 29, 2023 17:21

Update configSettingID from field to method.

dd998e4

This is to support how the new XDR is generated.

Minor updates to switch statements with CONFIG_SETTING.

79ce62b

Prohibit removing CONFIG_SETTING ledger entries.

89f69b5

While this can be supported from the ledger standpoint, I think that semantically these shouldn't be removed and hence it's better to introduce some harness to make sure we don't accidentally remove it.

Update the tests to use the new utilities and cover special handling …

a3cfa1c

…of `CONFIG_SETTING` entries.

sisuresh force-pushed the config_setting_le branch from ed002c2 to 1b2f664 Compare March 30, 2023 00:22

sisuresh added 4 commits March 29, 2023 17:41

ConfigSettingEntry upgrades

deb2167

Update tests

eb11a45

Fix flaky test

2bf6b63

Specify that createUpgradesFor takes the lcl header

e54282e

sisuresh added 4 commits March 29, 2023 17:41

Use debug json for upgrade info

91e6ea9

Ignore invalid upgrades instead of throwing an exception

cc4ce88

Add docs

25fe5ac

Update xdr

27bf0bd

sisuresh force-pushed the config_setting_le branch from 1b2f664 to 27bf0bd Compare March 30, 2023 00:42

sisuresh added 2 commits March 30, 2023 14:04

Add check rust vs c++ xdr size check

568a5a7

Bump env

cd3d956

sisuresh force-pushed the config_setting_le branch from 29f2490 to cd3d956 Compare March 30, 2023 22:35

latobarita merged commit d21f6a9 into stellar:master Apr 6, 2023

This was referenced Apr 7, 2023

Soroban: implement generalized network settings #3646

Closed

Add a function that can store configuration ledger entries stellar/rs-soroban-env#664

Closed

Conversation

sisuresh commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sisuresh Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MonsieurNicolas commented Mar 29, 2023

Uh oh!

sisuresh commented Mar 30, 2023

Uh oh!

MonsieurNicolas commented Mar 30, 2023

Uh oh!

sisuresh commented Mar 30, 2023

Uh oh!

sisuresh commented Mar 30, 2023

Uh oh!

graydon commented Apr 6, 2023

Uh oh!

graydon commented Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sisuresh commented Mar 9, 2023 •

edited

Loading

sisuresh Mar 23, 2023 •

edited

Loading