xdr: Add random XDR generator #3523

tamirms · 2021-04-01T17:29:52Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with name of package that is most changed in the PR, ex.
services/friendbot, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've updated the relevant CHANGELOG (here for Horizon) if
needed with deprecations, added features, breaking changes, and DB schema changes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

What

Close #3443

This commit introduces the randxdr package to the monorepo. randxdr is used for generating XDR values with random data.

randxdr was built using https://github.com/xdrpp/goxdr . Check out this presentation https://drive.google.com/file/d/1NG9aTGlf3yKMfIHdxXeGMpMNMDkOfGre/view to see how goxdr is better than other xdr compilers.

Why

The primary motivation behind building such a tool is that we could use it to generate a corpus for fuzz testing. We could also potentially integrate this tool into a property testing framework. This tool would be particularly helpful when working on protocol upgrades which require us to update the Stellar XDR definitions (e.g. #3430)

Known limitations

introduces another dependency to the monorepo ( https://github.com/xdrpp/goxdr )
introduces another autogenerated xdr go file ( gxdr/xdr_generated.go ) when we aleady xdr/xdr_generated.go

Counterpoint to (1): I think goxdr has no external dependencies itself.
Counterpoint to (2): I added a script to ensure that gxdr/xdr_generated.go is always in sync with the xdr/*.x files

.circleci/config.yml

paulbellamy · 2021-04-07T16:07:17Z

randxdr/marshaller.go

+//
+// Note that randMarshaller is stateful because of how union types are handled.
+// Therefore Marshal() should not be used concurrently.
+func (rm *randMarshaller) Marshal(field string, i goxdr.XdrType) {


Nit: I feel like filepath.Walk would be a better name and approach, than the Marshal/XdrRecurse combo, but that's more of a goxdr pull request, so, c'est la vie.

cc @stanford-scs

paulbellamy · 2021-04-07T16:09:59Z

randxdr/marshaller.go

+			bound = rm.maxBytesSize
+		}
+		bound++
+		bs := make([]byte, rm.rand.Uint32()%bound)


Could this OOM-kill the tests if it tries to claim 4GB of ram? Good to test though... 🤔

Ah, no the maxBytesSize is 256. But, should we actually test larger values? Are they possible?

This is configured by the MaxBytesSize field in Generator. 256 is an arbitrary default so I'm not sure if it's the best one

I agree we should test larger values - up to what's possible (I guess it's MAX_INT64 for xdr). Should be probably set to smaller values in envs with smaller memory (like Circle).

randxdr/marshaller.go

randxdr/presets.go

randxdr/generator.go

bartekn · 2021-04-07T16:42:53Z

randxdr/marshaller.go

+			bound = rm.maxBytesSize
+		}
+		bound++
+		bs := make([]byte, rm.rand.Uint32()%bound)


I agree we should test larger values - up to what's possible (I guess it's MAX_INT64 for xdr). Should be probably set to smaller values in envs with smaller memory (like Circle).

bartekn · 2021-04-07T16:46:32Z

services/horizon/internal/ingest/processors/offers_processor_test.go

+		gen.Next(
+			shape,
+			[]randxdr.Preset{
+				{randxdr.FieldEquals("type"), randxdr.SetU32(gxdr.LEDGER_ENTRY_CREATED.GetU32())},


We may want to generate updated and remove changes in the future. Do you have a plan how to do that?

I think you can do it in two steps. First generate a state entry containing an offer. For the update case generate an updated entry and preset the offer so it has the same id, allow the other offer fields to be random. In the remove case it would be similar except the type of the 2nd entry would be removed instead of updated

It could be interesting to model a state-machine around an account (e.g. create account, send a txn, updateData, etc) then generate sequences by a random walk through the state machine transitions.

bartekn · 2021-04-07T16:48:44Z

services/horizon/internal/ingest/processors/offers_processor_test.go

+				{randxdr.FieldEquals("created.lastModifiedLedgerSeq"), randxdr.SetPositiveNum32()},
+				{randxdr.FieldEquals("created.data.offer.amount"), randxdr.SetPositiveNum64()},
+				{randxdr.FieldEquals("created.data.offer.price.n"), randxdr.SetPositiveNum32()},
+				{randxdr.FieldEquals("created.data.offer.price.d"), randxdr.SetPositiveNum32()},


This is cool but we need to remove cases that would not pass Stellar-Core validation (like flags - basically reimplement doCheckValid methods from Core here). We should probably also generate only offer changes (or at least set probability of offer change to something like 90%).

I'm not sure we should do more than the bare minimum of validation because it will make our ingestion implementation more robust. If we do have any assumptions implicit in the ingestion code doing this type of testing will help make those assumptions more explicit

I understand what you mean. I think this is basically more validation vs more false positives dillema.

bartekn · 2021-04-07T16:51:32Z

xdr/json_test.go

+			shape,
+			[]randxdr.Preset{
+				{randxdr.IsPtr, randxdr.SetPtrToPresent},
+			},


As in offers test: we should make sure that it does not generate objects that wouldn't be accepted by Core. Ex. AbsBefore < 0.

Since we're testing if the code crashes / produces an error I don't think it's necessary to restrict the inputs more than the bare minimum

IMO, it should generate anything which could make it through the parser, then optionally be restricted down (where needed) to just stuff that makes sense.

bartekn

LGTM. I think it's worth doing more in terms of validation to decrease number of false positives but maybe let's check if we get too many of them.

tamirms force-pushed the random-xdr branch 4 times, most recently from 997dcb2 to 66513bd Compare April 1, 2021 20:22

Add random XDR generator

4e09c0a

tamirms force-pushed the random-xdr branch from 66513bd to 4e09c0a Compare April 2, 2021 09:14

tamirms marked this pull request as ready for review April 2, 2021 09:35

tamirms requested review from stanford-scs and a team April 2, 2021 09:36

tamirms changed the title ~~Add random XDR generator~~ xdr: Add random XDR generator Apr 2, 2021

paulbellamy approved these changes Apr 7, 2021

View reviewed changes

bartekn reviewed Apr 7, 2021

View reviewed changes

bartekn approved these changes Apr 8, 2021

View reviewed changes

tamirms and others added 3 commits April 10, 2021 09:10

Generate XDR in gogenerate.sh

ca97b07

Adjust default settings

f3e8573

Merge branch 'master' into random-xdr

4474e5d

tamirms merged commit 60f3afa into stellar:master Apr 12, 2021

tamirms deleted the random-xdr branch April 12, 2021 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdr: Add random XDR generator #3523

xdr: Add random XDR generator #3523

tamirms commented Apr 1, 2021 •

edited

Loading

paulbellamy Apr 7, 2021

tamirms Apr 12, 2021

paulbellamy Apr 7, 2021

paulbellamy Apr 7, 2021

tamirms Apr 7, 2021

bartekn Apr 7, 2021

bartekn Apr 7, 2021

bartekn Apr 7, 2021

tamirms Apr 7, 2021

paulbellamy Apr 8, 2021

bartekn Apr 7, 2021

tamirms Apr 7, 2021

bartekn Apr 8, 2021

bartekn Apr 7, 2021

tamirms Apr 7, 2021

paulbellamy Apr 8, 2021 •

edited

Loading

bartekn left a comment

xdr: Add random XDR generator #3523

xdr: Add random XDR generator #3523

Conversation

tamirms commented Apr 1, 2021 • edited Loading

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbellamy Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

bartekn left a comment

Choose a reason for hiding this comment

tamirms commented Apr 1, 2021 •

edited

Loading

paulbellamy Apr 8, 2021 •

edited

Loading