Amendment: ExpandedSignerList #4097

RichardAH · 2022-02-10T14:20:39Z

High Level Overview of Change

An amendment ExpandedSignerList, which if accepted, would:

Expand the maximum signer list to 32 entries
Allow the optional specification of 32 byte tags with each signer list entry

Context of Change

The motivation for this amendment is two fold:

Large organisations may have extensive signer lists which require more than 8 keys and may benefit from tags (for example to identify who is in possession of each key.)
Layer two smart contract solutions such as Evernode-hotpocket, codius and others gain a security benefit for a common use-case:

Often it is useful for layer 2 contracts to control layer 1 accounts, from which they send and receive tokens and currency. To do this, the servers that collectively run the layer 2 contract must hold private keys. The more keys required to sign a layer 1 transaction the harder it is for a third party to compromise the layer 1 account.

Remarks

This amendment PR includes a small but important refactor:
The Rules class has been extracted from ReadView.h/cpp and placed in its own files (Rules.h/cpp). A Rules reference may now be more easily passed in the codebase, including for example to signature verification routines (which makes sense because signature verification can be altered by amendments).

At @nbougalis's suggestion serialised field sfWalletLocator was reused as the uint256 for the optional tag in each SignerListEntry.

The ability to add tags to SignerListEntries required a change to the InnerObjectFormats definition. This object definition is used both in ltSIGNER_LIST and in ttSIGNER_LIST_SET. Thus care must be taken at the SetSignerList transactor to ensure tags cannot be set before amendment activation.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Test Plan

MultSign_test.cpp has been updated with a new test for the tags and additional signers (up to 32).

In addition I have tested using xrpl.js to sign transactions that:

Set signer list with and without tags
Set signer list of 8 (success) and 9 (fail) entries when amendment is not active
Set signer list of 8 (success), 9 (success), 32 (success) and 33 (fail) entries when amendment is active.

I have not included these tests anywhere, I encourage code reviewer to write own tests to avoid reliance on one set of tests.

For reference here is the JSON format for Signer Entries with tags:

{                                                                                                      
    "SignerEntry": {                                                                                   
        "Account": "rsA2LpzuawewSBQXkiju3YQTMzW13pAAdW",                                               
        "SignerWeight": 2,                                                                             
        "WalletLocator": "CAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFECAFE"            
    }                                                                                                  
},

RichardAH · 2022-02-10T15:07:04Z

updated: ran levelization and patched inconsistency between clang-format-10 (CI) and 13 (my version)

nbougalis

The code looks fine (it fails GRPC unit tests; that should be fixed prior to merging) and I don't think that increasing the number of signers is likely to have a significant impact on the ledger.

Left a comment about the sfWalletLocator field, and would like to see others chime in on this too.

nbougalis · 2022-02-11T18:13:20Z

src/ripple/ledger/impl/Rules.cpp

+
+namespace ripple {
+
+class Rules::Impl


I know you just moved this here and it was already present, but we should think about migrating away from the PIMPL idiom. It adds nothing, in this case.

Agree, but changing that was out of scope for this amendment (and in fact doesn't require an amendment).

nbougalis · 2022-02-11T18:21:40Z

src/ripple/protocol/impl/InnerObjectFormats.cpp

@@ -28,6 +28,7 @@ InnerObjectFormats::InnerObjectFormats()
        {
            {sfAccount, soeREQUIRED},
            {sfSignerWeight, soeREQUIRED},
+            {sfWalletLocator, soeOPTIONAL},


I understand why you want this field and concede that it's useful, but it may be slightly controversial in terms of size.

So is a 256-bit value really needed? Could we get away with something smaller? I think your original suggestion was to use a 64-bit tag. I suspect that would be fine, but maybe a little too constrained for what you want to do. 128 bits is probably sufficient, especially if you use it as an RFC-4122 UUID (likely V1, V4 or V5, depending on the usecase).

I am not overly concerned and I think it's fine as is, but I do think it's worth at least having a discussion about this to ensure the field is "right-sized".

Reusing an existing (essentially unused) serialized field is great because it makes it backwards compatible with old codecs. So if someone proposes to change the size of the tag they should also propose an existing sfField that matches their proposed size.

In my opinion reusing a pre-existing field is, in this case, probably a false economy. This commit will be bound together with many others in a release. At least one of the other commits in the release will likely change the codec. So having this particular commit avoid changing the codec does not prevent the codec from changing in a release.

Most programmers agree that names are hard. Giving a field the wrong name or a misleading name will only make the XRP Ledger even harder to use correctly.

My personal opinion is that you should pick the right sized SField type and select a truly meaningful name for the field. Proceed with that. If you are truly sold that WalletLocator is the best name you can come up with then I'd love to hear that explanation. Thanks.

trueIoV · 2022-02-11T23:29:36Z

Hi RichardAH, why not increase the number of signers to 100 in this case?
Especially since KAVA once stated that it could not integrate XRPL precisely because they need 100 signatures and no less.
Thanks.

RichardAH · 2022-02-12T08:24:54Z

@trueIoV 128 is possible but there should be some debate about the "right" number. Too many is an attack vector as follows:
Suppose we allow 65536 signatures and someone continually submits txns with 65535 correct signatures and 1 incorrect signature at the end. The nodes they send these txns to have to check all 65536 signatures and then discard the txn without billing any account on-ledger. This vector could be mitigated in several ways, one of which is to allow a fee claim failure to occur on any multisig transaction with more than X correct signatures, where X is an absolute number not relative to the total.

nbougalis · 2022-02-27T02:18:26Z

Hi RichardAH, why not increase the number of signers to 100 in this case? Especially since KAVA once stated that it could not integrate XRPL precisely because they need 100 signatures and no less. Thanks.

I don't know what Kava's rationale for "100 signatures and no less" is, but it smells a little like snake-oil; absent an actual, demonstrable need for such a high number of signatures and a rational explanation for why alternative solutions (e.g. Ed25519 threshold signatures) aren't sufficient, I don't think we should expand the signer list by that much.

Engineering is about trade-offs and especially in the case of a system like the XRPL I think we should be frugal in the 'cost' that participants are forced to bear in terms of computing resources, in the absence of a compelling reason for a feature. If that means some entity can't integrate... well, that's too bad.

ximinez

I took a quick look at these changes, and I don't have any feedback about the logic, but I did leave some comments with changes that will improve code safety, clarity, and readability.

src/ripple/app/tx/impl/SetSignerList.cpp

src/ripple/app/tx/impl/SignerEntries.cpp

src/ripple/protocol/STTx.h

src/test/jtx/impl/multisign.cpp

src/test/jtx/multisign.h

RichardAH · 2022-03-04T05:33:10Z

I’ll make these changes today and push an update

nbougalis · 2022-03-08T03:19:12Z

@RichardAH: I would suggest rethinking the 256-bit wallet locator value. Again, I'm not opposed to it, but can we get away with a 128-bit value? If you want to use it as a ledger locator, then the answer is no, but then WalletLocator isn't a good fit either. UserData or somesuch is probably a better fit.

I also think that some exposition, informing the motivation or possible uses behind this field might be helpful. I know you have specific use cases in mind, but others may not know what they are and that may make it more difficult.

RichardAH · 2022-03-08T07:06:48Z

@RichardAH: I would suggest rethinking the 256-bit wallet locator value. Again, I'm not opposed to it, but can we get away with a 128-bit value? If you want to use it as a ledger locator, then the answer is no, but then WalletLocator isn't a good fit either. UserData or somesuch is probably a better fit.

I also think that some exposition, informing the motivation or possible uses behind this field might be helpful. I know you have specific use cases in mind, but others may not know what they are and that may make it more difficult.

I have been thinking about whether or not 32 bytes of key identifier is the “right amount” and let me make the case why it is now:

In a large organisation such as a university or government department, assets including HSMs are typically assigned a 128 bit uuid which is printed on the asset. Good, but that only tells you which asset not where to find the asset. Leaving some extra bytes free to describe a place and building plus the asset number would make sense.

In the case of a layer two network which is trying to use the tag to find which servers in its cluster can sign: anything less than 32 bytes basically precludes using ipv6. It is the same problem as above. You could fit exactly one ipv6 address into a 128 bit field with nothing left over for extra information (like for example a port or protocol).

For these reasons I think the we should not reduce the size of the tag. If you are concerned about the data storage then we should instead increase ownercount per tag

wojake · 2022-03-17T14:34:38Z

I have a question related to sidechains, since door accounts can now have ~32 federators participating after this amendment is passed, would it be appropriate to create a new type of account for door accounts, perhaps we could create a new setting for this?

Rationale: Users are able to confidently indicate if an account is a door account, this would avoid any confusion or doubts as it won't look like a 'normal' account.

RichardAH · 2022-03-17T15:56:45Z

@wojake if anyone can create and mark an account in such a way how does it help users verify anything?

wojake · 2022-03-18T04:03:55Z

Yikes, I didn't think about that (my apologies). Currently, there's no way to verify if an account is a door account, so this is pointless if everyone can mark their account as a door account.
Unless there's a way to verify if an account is a door account on-chain, I don't think my proposal makes sense.

scottschurr

There are some problems in this pull request that should be addressed before it is accepted.

The Rules changes damage levelization. That needs to be fixed. Here is a commit that shows a possible solution: scottschurr@edf3aa5. Feel free to cherry-pick that commit if you would like to use it.
This commit causes the KnownFormatsToGRPC unit test to fail. The right way to fix that is to update the GRPC serialization code. Here is a commit that fixes the problem and makes the unit test pass: scottschurr@6fd72f6 Feel free to cherry-pick that commit if you would like.
With this pull request the MultiSign unit test no longer tests the pre-ExtendedSignerList functionality. The MultiSign unit test should be modified so it tests the old functionality at least until the ExpandedSignerList amendment has been enabled on the network. Here's a commit showing one way to achieve that change: scottschurr@b176359 Feel free to cherry-pick that commit if you would like.

I have an additional concern that is, however, not a blocker. My suspicion is that reusing the WalletLocator locator SField is a false economy. I think you are better off if you carefully pick a name that represents your intended use and then introduce that as a new SField with the right name. If you can argue that WalletLocator is really a good name for the intended use then I can go with that.

scottschurr · 2022-03-25T17:35:22Z

Builds/levelization/results/loops.txt

@@ -40,6 +40,9 @@ Loop: ripple.basics ripple.rpc
 Loop: ripple.core ripple.net
  ripple.net > ripple.core

+Loop: ripple.ledger ripple.protocol
+  ripple.ledger > ripple.protocol
+


As shown in this file, the changes made to Rules in this pull request hurt our already tenuous levelization. There's no question we should be taking other steps to improve the levelization situation, but we should at least avoid introducing commits that make the situation worse.

Fortunately, by reworking the Rules portion of this commit, the levelization problems introduced here can be fixed. One way it could be done is shown in this commit: https://github.com/ripple/rippled_ci/commit/edf3aa5dec316c477223cb67fd5dc4fdb51accdc

Feel free to cherry pick that commit in order to fix the levelization problem. I'm sure there are other viable ways to fix the problem if you prefer another approach.

scottschurr · 2022-03-25T17:49:08Z

src/test/app/MultiSign_test.cpp

@@ -1537,6 +1679,7 @@ class MultiSign_test : public beast::unit_test::suite
        test_multisigningMultisigner(features);
        test_signForHash(features);
        test_signersWithTickets(features);
+        test_signersWithTags(features);


I really appreciate you adding unit tests for the new functionality. Thank you. However, the way in which the unit tests were added results in the pre-ExpandedSignerList code no longer being tested. That's easy to fix. Here's a commit that shows one way to do so: scottschurr@b176359 Feel free to cherry-pick that commit if you would like.

In my opinion continuing to test the pre-ExpandedSignerList code is a requirement before this pull request can be merged.

scottschurr · 2022-03-25T20:06:24Z

src/ripple/protocol/impl/InnerObjectFormats.cpp

@@ -28,6 +28,7 @@ InnerObjectFormats::InnerObjectFormats()
        {
            {sfAccount, soeREQUIRED},
            {sfSignerWeight, soeREQUIRED},
+            {sfWalletLocator, soeOPTIONAL},


In my opinion reusing a pre-existing field is, in this case, probably a false economy. This commit will be bound together with many others in a release. At least one of the other commits in the release will likely change the codec. So having this particular commit avoid changing the codec does not prevent the codec from changing in a release.

Most programmers agree that names are hard. Giving a field the wrong name or a misleading name will only make the XRP Ledger even harder to use correctly.

My personal opinion is that you should pick the right sized SField type and select a truly meaningful name for the field. Proceed with that. If you are truly sold that WalletLocator is the best name you can come up with then I'd love to hear that explanation. Thanks.

RichardAH · 2022-03-29T12:59:29Z

Thanks for the review. I went ahead and squashed your commits into mine and added you as a co-author (seemed like the right approach to avoid too many commits for such a small amendment)

RichardAH · 2022-03-30T07:18:09Z

I have an additional concern that is, however, not a blocker. My suspicion is that reusing the WalletLocator locator SField is a false economy. I think you are better off if you carefully pick a name that represents your intended use and then introduce that as a new SField with the right name. If you can argue that WalletLocator is really a good name for the intended use then I can go with that.

I somewhat agree with you on this. The main advantage to its reuse is that old codecs can serialize and unserialize it. Which means legacy multisign tools can be trivially updated to use tags. I also do think WalletLocator is not an entirely inaccurate description. Tag is much less descriptive, so probably we would end up with something like KeyDescription or KeyLocation. At the end of the day it's a toss-up between nice backwards compatibility with existing codecs and a slightly incongruous name. I would lean toward keeping WalletLocator. I'd also add this is a niche feature, and almost no one will use or see it (relative to general use of the XRPL) so being pretty isn't important.

scottschurr

@RichardAH, thanks for your explanation for why you prefer WalletLocator for the added SField. I can go with that motivation. BTW, it was nice of you to flag me as a coauthor, but really not necessary. I consider my contribution to simply be part of the code review process.

Changes look good. The CI failures that I see are the result of ongoing CI problems, not a result of this pull request.

ximinez

Looks good!

RichardAH · 2022-04-11T11:29:16Z

(Rebased on most recent develop)

The amendment increases the maximum sign of an account's signer list from 8 to 32. Like all new features, the associated amendment is configured with a default vote of "no" and server operators will have to vote for it explicitly if they believe it is useful.

RichardAH force-pushed the ExpandedSignerList branch from 88c7800 to c5752e0 Compare February 10, 2022 15:05

nbougalis requested a review from scottschurr February 11, 2022 18:08

nbougalis approved these changes Feb 11, 2022

View reviewed changes

ximinez requested changes Mar 3, 2022

View reviewed changes

RichardAH force-pushed the ExpandedSignerList branch from c5752e0 to 33c236b Compare March 4, 2022 09:58

scottschurr suggested changes Mar 25, 2022

View reviewed changes

RichardAH force-pushed the ExpandedSignerList branch from 33c236b to f38603f Compare March 29, 2022 12:57

scottschurr approved these changes Mar 30, 2022

View reviewed changes

RichardAH requested a review from ximinez March 30, 2022 20:35

ximinez approved these changes Apr 6, 2022

View reviewed changes

scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Apr 6, 2022

RichardAH mentioned this pull request Apr 7, 2022

Proposed 1.9.0 #4140

Merged

Amendment: ExpandedSignerList

a2f9d78

RichardAH force-pushed the ExpandedSignerList branch from 3feea66 to a2f9d78 Compare April 11, 2022 11:19

This was referenced May 10, 2022

Propose 1.9.1-b1 #4158

Closed

Proposed 1.9.1-b1 #4161

Merged

manojsdoshi closed this in #4161 May 11, 2022

intelliot mentioned this pull request May 23, 2022

Known Amendments updates for 1.9.x XRPLF/xrpl-dev-portal#1409

Merged

This was referenced Jun 14, 2022

add ExpandedSignerList amendment support XRPLF/xrpl.js#2026

Merged

add ExpandedSignerList amendment support XRPLF/xrpl-py#406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amendment: ExpandedSignerList #4097

Amendment: ExpandedSignerList #4097

RichardAH commented Feb 10, 2022 •

edited

Loading

RichardAH commented Feb 10, 2022

nbougalis left a comment •

edited

Loading

nbougalis Feb 11, 2022

RichardAH Feb 12, 2022

nbougalis Feb 11, 2022

RichardAH Feb 12, 2022

scottschurr Mar 25, 2022

trueIoV commented Feb 11, 2022

RichardAH commented Feb 12, 2022 •

edited

Loading

nbougalis commented Feb 27, 2022

ximinez left a comment

RichardAH commented Mar 4, 2022

nbougalis commented Mar 8, 2022

RichardAH commented Mar 8, 2022

wojake commented Mar 17, 2022

RichardAH commented Mar 17, 2022

wojake commented Mar 18, 2022

scottschurr left a comment

scottschurr Mar 25, 2022

scottschurr Mar 25, 2022

scottschurr Mar 25, 2022

RichardAH commented Mar 29, 2022

RichardAH commented Mar 30, 2022

scottschurr left a comment

ximinez left a comment

RichardAH commented Apr 11, 2022

Amendment: ExpandedSignerList #4097

Amendment: ExpandedSignerList #4097

Conversation

RichardAH commented Feb 10, 2022 • edited Loading

High Level Overview of Change

Context of Change

Remarks

Type of Change

Test Plan

RichardAH commented Feb 10, 2022

nbougalis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueIoV commented Feb 11, 2022

RichardAH commented Feb 12, 2022 • edited Loading

nbougalis commented Feb 27, 2022

ximinez left a comment

Choose a reason for hiding this comment

RichardAH commented Mar 4, 2022

nbougalis commented Mar 8, 2022

RichardAH commented Mar 8, 2022

wojake commented Mar 17, 2022

RichardAH commented Mar 17, 2022

wojake commented Mar 18, 2022

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardAH commented Mar 29, 2022

RichardAH commented Mar 30, 2022

scottschurr left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

RichardAH commented Apr 11, 2022

RichardAH commented Feb 10, 2022 •

edited

Loading

nbougalis left a comment •

edited

Loading

RichardAH commented Feb 12, 2022 •

edited

Loading