Proposed 1.9.2-rc1 #4219

nbougalis · 2022-06-30T16:29:03Z

Roll-up PR for 1.9.2-rc, the first release candidate for 1.9.2, which introduces a fix for the amendment voting persistence issue described in #4220, as well as a fix for "negative offers" for NFTs.

Primarily, this release will introduce a new amendment, NonFungibleTokensV1_1 which is a combination of three amendments: NonFungibleTokensV1, fixNFTokenDirV1 and fixNFTokenNegOffer. The maintainers of the reference implementation of the protocol recommend that operators that wish to vote for activation of the XLS-20 NFT enhancements vote in favor of NonFungibleTokensV1_1 instead of NonFungibleTokensV1.

If merged, this release will:

nbougalis · 2022-06-30T19:30:13Z

Important Note for Reviewers, Maintainers and other interested parties:

This PR includes commit c4d707a, which changes the default vote for two amendments: the previously introduced fixNFTokenDirV1 and fixNFTokenNegOffer (introduced with PR #4215).

Prior to this commit, the two fix amendments had a default vote of no and this changes the vote to yes.

Considering that both of these amendments correct bugs (as opposed to introducing new features) and that, even if activated, their code is dormant and will remain gated under the featureNonFungibleTokensV1 amendment (which defaulted and continues to default to a no) the impact of this change should be minimal. The primary rationale for this change is to add an additional layer of protection, so as to avoid the accidental activation of the featureNonFungibleTokensV1 without these important fixes.

Note that, while not presently used outside the inactive NFT code, the fixes introduced in the fixNFTokenDirV1 amendment may prove useful independently of whether NFT support is added, as the new-style page structures can be leveraged to build a more compact and efficient method for dealing with owner directories.

rippleitinnz · 2022-06-30T20:21:34Z

While I'm in favour of this, a default YES vote would initiate the timer once sufficient are updated and would cause an early amendment block if sufficient didn't change their vote to no. This may be a problem.

nbougalis · 2022-06-30T20:48:27Z

While I'm in favour of this, a default YES vote would initiate the timer once sufficient are updated and would cause an early amendment block if sufficient didn't change their vote to no. This may be a problem.

This is a good point to make: these amendments, despite being a "nop" until the featureNonFungibleTokensV1 activates, will amendment block anyone not on 1.9.2. It's a bit of a pickle, to be sure, but that's the same as with any amendment really.

We should rethink the way amendments are proposed how they work, especially vis-à-vis amendments that are dependent on some other amendment.

rippleitinnz · 2022-06-30T20:53:46Z

Ultimately, the resolution would be to have trusted validators who were timely, proactive in discussing and engaging with the voting process on our UNL lists.

nbougalis · 2022-07-03T13:28:03Z

So, obviously there's some controversy about setting the two fix amendments to "default yet". I've noticed some vigorous conversations on Twitter and the Mattermost channels, perhaps in the Discord too.

I think this is important to discuss publicly and transparently.

As I wrote earlier, "[c]onsidering that both of these amendments correct bugs (as opposed to introducing new features) and that, even if activated, their code is dormant and will remain gated under the featureNonFungibleTokensV1 amendment (which defaulted and continues to default to a no) the impact of this change should be minimal."

I think my analysis is reasonable but my conclusion is not: the impact is not minimal, in the sense that if these amendments activate, anyone running 1.8.x, 1.9.0 or 1.9.1 will become amendment blocked for no real reason: the amendments don't do anything.

Broadly, these raises some questions about the flexibility of the amendment code and whether it should be improved to allow, inter alia, the possibility of an amendment that activates but doesn't amendment block anyone, because it's dependent on another amendment. But I don't want to detract from the issue at hand: changing the default vote for the fixes.

Again, I believe this is a change that I can defend rationally, but I understand that it may be controversial, especially given the broader context.

I hope people will chime in here and we can have a productive and meaningful conversation that results in consensus on the way forward, regardless of what that consensus is.

Thanks!

Mwni · 2022-07-03T13:53:55Z

Why don't we add the option for an amendment to default on the setting of another amendment?
So the example would be, the two fixes would have the default = nonFungibleTokensV2.enabled. That way, unconscientious validator operators would be opted out of all NFT related amendments, while those who care still have full control.

nbougalis · 2022-07-03T15:38:56Z

Why don't we add the option for an amendment to default on the setting of another amendment? So the example would be, the two fixes would have the default = nonFungibleTokensV2.enabled. That way, unconscientious validator operators would be opted out of all NFT related amendments, while those who care still have full control.

Interesting suggestion. We'd discussed something like this before. I think having "conditional amendments" is a good idea: "A requires B" or "C implies D, E and F" but we don't have that and coding it would, itself, require an amendment. So chicken and egg for this case. But certainly worth considering.

MarkusTeufelberger · 2022-07-03T19:05:27Z

Well, servers could just not propose Amendment A until they see also Amendment B supported on the network by default if A would be broken without B.

danielwwf · 2022-07-03T23:55:10Z

While I am somehow in favour of a default yes, if dependencies exist, we shouldn't do it this time. Our main target, for now, should be getting as many nodes updated as possible, as soon as possible. From a dUNL standpoint we should be fine, its the rest of the network we need to worry about. As mentioned before, we aren't even sure if outdated nodes will have an impact but the risk is too high from my point of view.

Broadly, these raises some questions about the flexibility of the amendment code and whether it should be improved to allow, inter alia, the possibility of an amendment that activates but doesn't amendment block anyone, because it's dependent on another amendment. But I don't want to detract from the issue at hand: changing the default vote for the fixes.

Moving forward that's something that needs to be looked into to avoid similar sitations.

gnurag · 2022-07-04T10:40:13Z

Nodes being amendment blocked for no real material reason (i.e. fixNFTokenDirV1 and fixNFTokenNegOffer activation) is a fair point. It makes sense to merge all NFToken related code+fixes into a single votable amendment.

Silkjaer · 2022-07-05T21:36:22Z

It makes sense to merge all NFToken related code+fixes into a single votable amendment.

I agree, as I believe it’s a lot easier for people to follow one voting process (instead of waiting for two fixes and main amendment). And then it sets a precedent for how to deal with similar situations in the future, without making changes to support dependency amendments or similar.

nbougalis · 2022-07-05T21:50:40Z

So, I'm trying to decide on what to do with this PR.

The team at Ripple is looking into Votes for amendments not persisting #4220 and I think we should understand what's happening there before we proceed here.
There's obviously been some concerns about commit c4d707a.

So, about c4d707a: we have the following options:

Leave the commit as is, defaulting the votes for the 2 fix amendments to yes; they will then default to yes. If either of the two fix amendments gain majority, operators will need to upgrade to at least 1.9.1 even though the fix amendments are effective no-ops until featureNonFungibleTokensV1 activates.
Remove the commit that defaults the votes for the 2 fix amendments to yes; they will default to no. If either of the two fix amendments gain majority, operators will need to upgrade to at least 1.9.1 even though the fix amendments are effective no-ops until featureNonFungibleTokensV1 activates.
Deprecate the three existing amendments (voting for them will do nothing) and introduce featureNonFungibleTokensV1.1 which achieves the same as featureNonFungibleTokensV1 and the two fix amendments. The new amendment will default to no.

Would appreciate feedback on what people think is the right next step here.

Silkjaer · 2022-07-05T21:54:02Z

Option 3

jscottbranson · 2022-07-05T21:56:35Z

I agree with @gnurag and @Silkjaer regarding deprecating the current amendments in favor of featureNonFungibleTokensV1.1, and I agree that the amendment should default to no.

danielwwf · 2022-07-06T00:02:04Z

I am in favour of Option 3

Mwni · 2022-07-06T00:14:20Z

Option 3

rippleitinnz · 2022-07-06T00:24:44Z

Option 3 is the most logical. Having said that, a voter who has voted to ACCEPT, who becomes unaware of any fix and the subsequent deprecation of the amendment, by it being replaced with a newer version, will have their vote recorded as REJECT despite them thinking they had voted to ENABLE.

Currently there is no way to know that a vote you have cast has been changed or removed without checking your voting status.

alloynetworks · 2022-07-06T04:50:53Z

I would go with option 3.

WietseWind · 2022-07-06T05:40:36Z

I would go for option 3.

jonaagenilsen · 2022-07-06T05:41:36Z

Option 3 😊

nixer89 · 2022-07-06T05:47:07Z

Option 3 makes the most sense for me 👌

wojake · 2022-07-06T06:13:05Z

Option 3 is best IMO since it compiles all the current NFT-related amendments into a single amendment providing a better decision making process for all rippled node operators.

Digifin · 2022-07-06T13:22:24Z

Option 3 sounds the best.

rswarthout · 2022-07-06T15:29:18Z

Option 3

ZerpCraft · 2022-07-06T15:29:37Z

Option 3 sounds like a winner!

shortthefomo · 2022-07-06T17:53:46Z

im good with option 3

calvincs · 2022-07-06T18:50:22Z

Option three looks like a solid choice to me.

ximinez · 2022-07-07T16:37:04Z

src/ripple/protocol/impl/Rules.cpp

+    // The functionality of the "NonFungibleTokensV1_1" amendment is
+    // precisely the functionality of the following three amendments
+    // so if their status is ever queried individually, we inject an
+    // extra check here to simplify the checking elsewhere.
+    if (feature == featureNonFungibleTokensV1 ||
+        feature == fixNFTokenNegOffer || feature == fixNFTokenDirV1)
+    {
+        if (impl_->enabled(featureNonFungibleTokensV1_1))
+            return true;
+    }
+


The only downside to doing it this way is that this check will need to be made on every call to enabled. It's a tiny downside, since it's a simple check. But what do you think of, instead of this check, you modify Change::applyAmendment. If the enabled feature is featureNonFungibleTokensV1_1, you add the other three to the amendments list, too?

rippled/src/ripple/app/tx/impl/Change.cpp

Lines 195 to 197 in 83aa1c1

// No flags, enable amendment

amendments.push_back(amendment);

amendmentObject->setFieldV256(sfAmendments, amendments);

I thought about this (and prepared a branch to do just that, actually) but a couple of people that I floated that idea by felt that it was weird, because it could result in an amendment that doesn't, on its own, have majority appearing to get arbitrarily enabled.

I do think that's a better option in the long term and will likely introduce a PR to make that possible, but I don't think it's the right option here.

ximinez · 2022-07-07T16:38:04Z

src/ripple/protocol/Feature.h

@@ -337,6 +337,7 @@ extern uint256 const featureNonFungibleTokensV1;
 extern uint256 const featureExpandedSignerList;
 extern uint256 const fixNFTokenDirV1;
 extern uint256 const fixNFTokenNegOffer;
+extern uint256 const featureNonFungibleTokensV1_1;


You need to increment numFeatures.

rippled/src/ripple/protocol/Feature.h

Line 77 in 83aa1c1

static constexpr std::size_t numFeatures = 49;

ximinez · 2022-07-07T16:38:44Z

src/ripple/protocol/impl/Rules.cpp

@@ -17,10 +17,10 @@
 */
 //==============================================================================

+#include <ripple/protocol/Feature.h>
+//#include <ripple/protocol/Indexes.h>


May as well remove the commented line.

ximinez · 2022-07-07T20:02:39Z

It also turns out that the NonFungibleTokensV1_1 breaks several unit tests. I pushed a commit that fixes them, as well as numFeatures. ximinez@ab5c809

nbougalis · 2022-07-11T17:09:22Z

Per the discussion here and in the mattermost chat, I reverted the commit that changed the default votes for the two fix amendments and have, instead, introduced a new amendment, NonFungibleTokensV1_1 which is a combination of three amendments: NonFungibleTokensV1, fixNFTokenDirV1 and fixNFTokenNegOffer.

Assuming that this is merged, it should be interpreted as a recommendation by the maintainers of the reference implementation of the protocol that any operators who wish to vote for activation of the XLS-20 NFT enhancements vote in favor of NonFungibleTokensV1_1 instead of NonFungibleTokensV1.

src/ripple/protocol/impl/Rules.cpp

The existing code properly parses the network_id parameter from the the configuration file, but it does not properly set up the code to use the value correctly. As a result the configured `network_id` is ignored.

ThreadSafetyAnalysis was used to identify race conditions in this file. This analysis was modivated by a (rare) crash while running unit tests. Add locks to Shard flagged by ThreadSafetyAnalysis

While there should never be a missing node when copying the SHAMap, rippled should not terminate when there's an error rotating the database. This patch aborts the database rotation rather than aborting rippled.

A few unit tests have historically generated a lot of noise to the console from log writes. This noise was not useful and made it harder to locate actual test failures. By changing the log level of these tests from - severities::kError to - severities::kDisabled it was possible to remove that noise coming from the logs.

The existing code would, incorrectly, allow negative amounts in offers for non-fungible tokens. Such offers would be handled very differently depending on the context: a direct offer would fail with an error code indicating an internal processing error, whereas brokered offers would improperly succeed. This commit introduces the `fixNFTokenNegOffer` amendment that detects such offers during creation and returns an appropriate error code. The commit also extends the existing code to allow for buy offers that contain a `Destination` field, so that a specific broker can be set in the offer.

An incorrect SQL query could cause the server to improperly configure its voting state after a restart; typically, this would manifest as an apparent failure to store a vote which the administrator of the server had configured. This commit fixes the broken SQL and ensures that amendment votes are properly reloaded post-restart and closes #4220.

The peer discovery protocol depends on peers exchanging messages listing IP addresses for other peers. Under normal circumstances, these messages should not be sent frequently; the existing code would track the earliest time a new message should be processed, but did not actually enforce that limit.

The XLS-20 implementation contained two bugs that would require the introduction of amendments. This complicates the adoption of XLS-20 by requiring a staggered amendment activation, first of the two fix amendments, followed by the `NonFungibleTokensV1` amendment. After consideration, the consensus among node operators is that the process should be simplified by the introduction of a new amendment that, if enabled, would behaves as if the `NonFungibleTokensV1` and the two fix amendments (`fixNFTokenDirV1` and `fixNFTokenNegOffer`) were activated at once. This commit implements this proposal; it does not introduce any new functionality or additional features, above and beyond that offered by the existing amendments.

The existing spinlock code, used to protect SHAMapInnerNode child lists, has a mistake that can allow the same child to be repeatedly locked under some circumstances. The bug was in the `SpinBitLock::lock` loop condition check and would result in the loop terminating early. This commit fixes this and further simplifies the lock loop making the correctness of the code easier to verify without sacrificing performance. It also promotes the spinlock class from an implementation detail to a more general purpose, easier to use lock class with clearer semantics. Two different lock types now allow developers to easily grab either a single spinlock from an a group of spinlocks (packed in an unsigned integer) or to grab all of the spinlocks at once. While this commit makes spinlocks more widely available to developers, they are rarely the best tool for the job. Use them judiciously and only after careful consideration.

seelabs

👍

The concerns raised have been addressed.

nixer89

looking good

ximinez previously requested changes Jul 7, 2022

View reviewed changes

Describe resolution for common SOCI-related build errors

3172a81

Properly handle incorrect port numbers in parseURL (fixes #4200)

d632f9f

scottschurr reviewed Jul 12, 2022

View reviewed changes

src/ripple/protocol/impl/Rules.cpp Outdated Show resolved Hide resolved

ximinez self-requested a review July 15, 2022 21:52

Chenna Keshava B S and others added 19 commits July 17, 2022 22:17

Correctly use the configured network_id parameter:

e46d2bc

The existing code properly parses the network_id parameter from the the configuration file, but it does not properly set up the code to use the value correctly. As a result the configured `network_id` is ignored.

Add support for clang's ThreadSafetyAnalysis

f55913d

Fix race conditions in shard:

8e6a0d4

ThreadSafetyAnalysis was used to identify race conditions in this file. This analysis was modivated by a (rare) crash while running unit tests. Add locks to Shard flagged by ThreadSafetyAnalysis

Catch missing node error when rotating database:

723733a

While there should never be a missing node when copying the SHAMap, rippled should not terminate when there's an error rotating the database. This patch aborts the database rotation rather than aborting rippled.

Fix bitwise or on boolean operands warning / error

1f75ba2

Lower the message level for missing optional doc components

18d4372

Lower log level of "addPathsForType" log message (fixes #4177)

ee60b16

Improve JSON sanitization in reporting mode

d458e99

Set cluster timer only when in a cluster

5e6728d

Improve STVector256 deserialization

9eb303f

Fix #4231 (wrong include guard) and an out-of-order construction warning

b95ca98

Fix a race condition during shutdown

22b4de2

Set version to 1.9.2-rc1

83faf43

seelabs approved these changes Jul 18, 2022

View reviewed changes

nbougalis requested review from nixer89 and removed request for ximinez July 19, 2022 06:12

nixer89 approved these changes Jul 19, 2022

View reviewed changes

nbougalis merged commit 83faf43 into XRPLF:develop Jul 19, 2022

nbougalis mentioned this pull request Jul 26, 2022

Proposed 1.9.2 #4252

Merged

	// No flags, enable amendment
	amendments.push_back(amendment);
	amendmentObject->setFieldV256(sfAmendments, amendments);

Proposed 1.9.2-rc1 #4219

Proposed 1.9.2-rc1 #4219

Conversation

nbougalis commented Jun 30, 2022 • edited Loading

nbougalis commented Jun 30, 2022

rippleitinnz commented Jun 30, 2022 • edited Loading

nbougalis commented Jun 30, 2022 • edited Loading

rippleitinnz commented Jun 30, 2022

nbougalis commented Jul 3, 2022

Mwni commented Jul 3, 2022

nbougalis commented Jul 3, 2022

MarkusTeufelberger commented Jul 3, 2022

danielwwf commented Jul 3, 2022

gnurag commented Jul 4, 2022

Silkjaer commented Jul 5, 2022

nbougalis commented Jul 5, 2022

Silkjaer commented Jul 5, 2022

jscottbranson commented Jul 5, 2022 • edited Loading

danielwwf commented Jul 6, 2022

Mwni commented Jul 6, 2022

rippleitinnz commented Jul 6, 2022 • edited Loading

alloynetworks commented Jul 6, 2022

WietseWind commented Jul 6, 2022

jonaagenilsen commented Jul 6, 2022 • edited Loading

nixer89 commented Jul 6, 2022

wojake commented Jul 6, 2022

Digifin commented Jul 6, 2022

rswarthout commented Jul 6, 2022

ZerpCraft commented Jul 6, 2022

shortthefomo commented Jul 6, 2022

calvincs commented Jul 6, 2022

ximinez Jul 7, 2022

Choose a reason for hiding this comment

nbougalis Jul 14, 2022

Choose a reason for hiding this comment

ximinez Jul 7, 2022

Choose a reason for hiding this comment

ximinez Jul 7, 2022

Choose a reason for hiding this comment

ximinez commented Jul 7, 2022

nbougalis commented Jul 11, 2022

seelabs left a comment

Choose a reason for hiding this comment

nixer89 left a comment

Choose a reason for hiding this comment

nbougalis commented Jun 30, 2022 •

edited

Loading

rippleitinnz commented Jun 30, 2022 •

edited

Loading

nbougalis commented Jun 30, 2022 •

edited

Loading

jscottbranson commented Jul 5, 2022 •

edited

Loading

rippleitinnz commented Jul 6, 2022 •

edited

Loading

jonaagenilsen commented Jul 6, 2022 •

edited

Loading