txnbuild: Fix bug in parsing MangeData xdr operations

[tamirms]

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 #2572

Fix bug which occurs when parsing transactions with manage data operations containing nil values.

It seems like this bug has been present in older versions of txnbuild. I ran the following test case on versions 2.2.0, 2.1.0, 2.0.0, 1.50, and 1.4.0 and they all produced a panic similar to what's described in #2572:

func TestParseNil(t *testing.T) {
	op := ManageData{
		Name:  "key",
		Value: []byte(nil), // <<<<<<<<<<<<<<<<<<<<
	}
	tx := Transaction{
		SourceAccount: &SimpleAccount{AccountID: "GDYHQK7C2OMUMU3I6553YDQ56RUQL2JFL6FPYOYGLDKS3QGXDC4SRLOW"},
		Operations:    []Operation{&op},
		Timebounds:    NewInfiniteTimeout(),
		Network: network.TestNetworkPassphrase,
	}
	txe, err := tx.BuildSignEncode()
	assert.NoError(t, err)
	parsed, err := TransactionFromXDR(txe)
	assert.NoError(t, err)
	t.Log(parsed)
}

I checked if there were other potential nil dereferences in the txnbuild package by running this search:

Tamirs-MBP:go tamirsen$ grep "\*."  txnbuild/*.go | grep -v _test.go | grep -v func
txnbuild/manage_data.go:                md.Value = *result.DataValue
txnbuild/set_options.go:        InflationDestination *string
txnbuild/set_options.go:        MasterWeight         *Threshold
txnbuild/set_options.go:        LowThreshold         *Threshold
txnbuild/set_options.go:        MediumThreshold      *Threshold
txnbuild/set_options.go:        HighThreshold        *Threshold
txnbuild/set_options.go:        HomeDomain           *string
txnbuild/set_options.go:        Signer               *Signer
txnbuild/set_options.go:                err = xdrAccountID.SetAddress(*so.InflationDestination)
txnbuild/set_options.go:                        if f&AccountFlag(*flags) != 0 {
txnbuild/set_options.go:                        if f&AccountFlag(*flags) != 0 {
txnbuild/set_options.go:                xdrWeight := xdr.Uint32(*so.MasterWeight)
txnbuild/set_options.go:                mw := Threshold(uint32(*weight))
txnbuild/set_options.go:                xdrThreshold := xdr.Uint32(*so.LowThreshold)
txnbuild/set_options.go:                lt := Threshold(uint32(*weight))
txnbuild/set_options.go:                xdrThreshold := xdr.Uint32(*so.MediumThreshold)
txnbuild/set_options.go:                mt := Threshold(uint32(*weight))
txnbuild/set_options.go:                xdrThreshold := xdr.Uint32(*so.HighThreshold)
txnbuild/set_options.go:                ht := Threshold(uint32(*weight))
txnbuild/set_options.go:                if len(*so.HomeDomain) > 32 {
txnbuild/set_options.go:                xdrHomeDomain := xdr.String32(*so.HomeDomain)
txnbuild/set_options.go:                domain := string(*xDomain)
txnbuild/transaction.go:*/
txnbuild/transaction.go:        kps ...*keypair.Full,
txnbuild/transaction.go:        var signers []*keypair.Full
txnbuild/transaction.go:                kpf, ok := kp.(*keypair.Full)
txnbuild/transaction.go:        *newTx = *t
txnbuild/transaction.go:        *newTx = *t
txnbuild/transaction.go:        inner      *Transaction
txnbuild/transaction.go:        *newTx = *t
txnbuild/transaction.go:        *newTx = *t
txnbuild/transaction.go:        *innerCopy = *t.inner
txnbuild/transaction.go:        simple  *Transaction
txnbuild/transaction.go:        feeBump *FeeBumpTransaction
txnbuild/transaction.go:                var innerTx *GenericTransaction
txnbuild/transaction.go:        Inner      *Transaction
txnbuild/transaction.go:                maxFee:     params.BaseFee * int64(len(params.Inner.operations)+1),
txnbuild/transaction.go:        *tx.inner = *params.Inner
txnbuild/transaction.go:        tx, err = tx.Sign(network, serverKP.(*keypair.Full))
txnbuild/transaction.go:        op, ok := operations[0].(*ManageData)

Afterwards, I looked at all the cases in txnbuild/set_options.go. They seemed fine because the pointer derefrences were guarded by nil checks.

[tamirms]

For future releases of txnbuild we can add a smoke test where we call TransactionFromXDR() on all transaction envelopes included an arbitrary pubnet ledger and check there are no errors.

[ire-and-curses]

Thanks for the quick turnaround on this!

[ire-and-curses]

I support releasing this immediately as a patch release.

[leighmcculloch]

👍 Solid fix.

[leighmcculloch]

I love that you set this to nil explicitly even though we don't need to. It makes the intention really clear.

[leighmcculloch]

It wouldn't hurt to add tests for the cases when the data value has a valid value non-empty and when it is valid empty (not nil). I don't see any tests for these situations either.