Improve STVector256 deserialization

[nbougalis]

The existing STVector256 is not very optimal: it requires a lot of unnecessary copying. This commit fixes that and improves detection improperly serialized values: now such values will result in an exception being raised (exceptions from that codepath are expected and properly handled).

High Level Overview of Change

Context of Change

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

[greg7mdp]

maybe using const_iterator = value_type const*;

[seelabs]

The NFTokenCancelOffer uses a STVector256 field. The old code may have accepted an incorrectly serialized sfNFTTokenOffers field where as the new code throws - which would make this transaction breaking. I'd be surprised if an incorrectly serialized transaction could ever make into the ledger, but I'm not sure I can prove it (code like this would assert in debug mode: https://github.com/nbougalis/rippled/blob/c497a62fb95c98a1e29f0eb0557ca36adcb89c7d/src/ripple/app/tx/impl/Transactor.cpp#L785-L802)

I think it's fine as-is, but I did want to bring this up.

[nbougalis]

Thanks for bringing this up. So two comments:

1. This cannot occur on the main network since the featureNonFungibleTokensV1 amendment isn't active, and so the NFTokenCancelOffer transaction cannot be executed.
2. In addition to the code you showed, the code will call sterilize on submitted transactions, which should detect this in some circumstances.

Given than we're planning to introduce another fix amendment to correct a minor issue identified with the XLS-20 implementation, which (for obvious reasons!) should be activated before XLS-20 implementation, I think my suggestion would be to include this code with this release. It doesn't need to be directly gated by the amendment, but if the fix amendment activates before featureNonFungibleTokensV1 (and again, it should), then this issue will never be able to trigger.

Apropos of this, I wonder what the performance implications of always executing the serialization/deserialization check that's currently only done if compiled with -DDEBUG would be. Also, I wonder whether this check more properly belongs in either preflight or preclaim (we ought to claim a fee if someone submits a broken transaction and entomb the transation in the ledger!).

There are good reasons to have it in operator() (i.e. during "application") too, but, as I said above, if this check fails, it would be better to tec out of there, instead of simply calling assert.

[seelabs]

Thanks for the clarifications. Yeah, definitely fine as-is. 👍

[greg7mdp]

The STVector256 class duplicates a lot of the std::vector interface. I think it would be simpler to declare it as:

class STVector256 : public STBase, public std::vector<uint256> { ...};

This would allow to remove all the duplicated vector APIs (and also get the newer ones like emplace :-))

[HowardHinnant]

I disagree with publicly inheriting from std::vector<uint256>. This would introduce an unwanted implicit conversion to std::vector<uint256>. The current API has an explicit conversion to std::vector<uint256>. It would also introduce implicit conversions between STVector256* and std::vector<uint256>*

[greg7mdp]

@HowardHinnant , why do you find the implicit conversion objectionable? If we are honest, the STVector256 is mainly a std::vector<uint256>. IMHO, the (incomplete) duplication of the vector interface is worse.

[HowardHinnant]

If we want an implicit conversion to vector, that is one thing. But the existing API has an explicit conversion. The distinction between implicit and explicit conversions is not small. It can have a very large impact on client code that goes well beyond just syntax.

We should not change the implementation to something more convenient without a full accounting of the desired API of the class. The priority of correctness is more important than the priority of ease of writing.

Today, the following code is a compile-time error:

   auto pST = new STVector256{};
   std::vector<uint256>* pv = pST;
   delete pv;

With the proposed change the above sequence becomes a run-time error. Compile-time errors are caught at compile-time. Run-time errors have the potential to be shipped and create problems for the XRP network.

[greg7mdp]

I'm not quite convinced that doing what I suggest is as dangerous as you make it to be, and I think there is also potential danger having extra code duplicating an API unnecessarily. But you seem to feel strongly about it, so let's leave it as is.

[nbougalis]

I'll be honest, I've thought about this before, @greg7mdp; eliminating some of the unnecessary wrapper functions appeals to me. However there are issues: not all std::vector operations are appropriate in an STVector256, and I do share some of @HowardHinnant's concerns.

I think it's worth thinking about this in the broader context of classes like STVector and STBlob but it's not something that should be done without careful consideration.

[scottschurr]

👍 LGTM.

[nbougalis]

@greg7mdp, I addressed the comment you left. If you wouldn't mind approving this PR, thanks.

[greg7mdp]

Thanks for addressing my comment Nik!