services/horizon: Use new ingestion data in `/accounts/{account_id}` #1868

bartekn · 2019-10-22T16:13:42Z

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.

Summary

This commit adds temporary code (to be removed in one of the future versions of Horizon) that compares account state in Stellar-Core and Horizon DB. If a difference is found it's logged with WARNING level.

Close #1766.
See #1726.

Known limitations & issues

It's possible that false alerts will happen because the old code does not use a DB transaction to get data from core DB. This means that new ledger can be applied during request is live.

abuiles

@bartekn Looks good! I have a doubt though, shouldn't we check if expingestion is enabled before comparing results from both sources?

Also can we use the new pattern so the new action is used only if exp-ingestion is turned on.

bartekn · 2019-10-28T16:54:02Z

shouldn't we check if expingestion is enabled before comparing results from both sources?

Good point, fixed and ready to review again.

Also can we use the new pattern so the new action is used only if exp-ingestion is turned on.

Not yet as we are still serving old data (and comparing with new data).

abuiles

@bartekn looks good! Can you add test for the new methods on Q?

services/horizon/internal/db2/history/account_data.go

services/horizon/internal/db2/history/account_signers.go

services/horizon/internal/db2/history/accounts.go

services/horizon/internal/db2/history/trust_lines.go

bartekn · 2019-10-28T18:20:19Z

@abuiles great point about tests! I somehow though that I was using methods from #1835 but it's not true obviously.

bartekn · 2019-10-30T14:50:01Z

@abuiles tests added. PTAL!

abuiles

@bartekn LGTM! I think there is a typo, where you have two err assertions one after the other one.

abuiles · 2019-10-30T15:03:29Z

services/horizon/internal/db2/history/account_data_test.go

+	assert.Equal(t, []byte(data1.DataValue), []byte(datas[0].Value))
+
+	assert.Equal(t, data2.DataName, xdr.String64(datas[1].Name))
+	assert.Equal(t, []byte(data2.DataValue), []byte(datas[1].Value))


nit: tt wraps assert, you could have tt.Assert( without having to pass t all the time.

abuiles · 2019-10-30T15:05:04Z

services/horizon/internal/db2/history/account_data_test.go

+	_, err = q.InsertAccountData(data2, 1235)
+	assert.NoError(t, err)
+
+	datas, err := q.GetAccountDataByAccountID(data1.AccountId.Address())


nit: data is already plural :), my workaround was to call this records, my mental model is that records are data from DB , which maps to Q'like structs

abuiles · 2019-10-30T15:09:46Z

services/horizon/internal/db2/history/account_signers_test.go

+	results, err := q.GetAccountSignersByAccountID(account)
+	tt.Assert.NoError(err)
+	tt.Assert.Len(results, 2)
+	tt.Assert.Equal(expected, results)


I've used this pattern too, something I think every time I do it is that this might break at some points because it's order dependent. I've been using the map, del and len assert approach. No strong feelings about this just an open thought -- I wonder what other patterns exists for this -- @leighmcculloch @tamirms thoughts?

In this case I think we should actually sort signers so tools like horizon-cmp don't have to deal with this issue too. EDIT: added sorting to GetAccountSignersByAccountID.

abuiles · 2019-10-30T15:18:54Z

services/horizon/internal/db2/history/accounts_test.go

+	tt.Assert.NoError(err)
+
+	resultAccount, err := q.GetAccountByID(account1.AccountId.Address())
+	assert.NoError(t, err)


@bartekn why the double assert?

Copy&paste issue.

bartekn · 2019-10-30T16:25:21Z

@abuiles PTAL!

abuiles

🚀

tamirms · 2019-10-30T16:47:52Z

services/horizon/internal/actions/account.go

+
+		// We send JSON bytes to compareAccountResults to prevent modifying
+		// `resource` in any way.
+		err = compareAccountResults(ctx, hq, c, addr)


should we check that both ingestion systems are at the same ledger before comparing results?

We do check this using LastModifiedLedger in accountResourcesEqual.

tamirms · 2019-10-30T16:51:28Z

services/horizon/internal/actions/account.go

+	}
+
+	if enableExperimentalIngestion {
+		c, err := json.Marshal(resource)


why not pass in the resource directly to compareAccountResults() ? you end up unmarshaling the bytes in compareAccountResults() so you could skip that step by sending the protocol.Account instance to compareAccountResults()

It's explained in the comments:

// We send JSON bytes to compareAccountResults to prevent modifying // `resource` in any way.

It's to minimize chances of rendering data from expingest instead of stellar-core by accident. The compare code will be removed when expingest goes to production.

bartekn added 2 commits October 22, 2019 17:56

services/horizon: Use new ingestion data in /accounts/{account_id}

0358cef

Start transaction

62a083e

bartekn requested review from abuiles, tamirms and ire-and-curses October 22, 2019 16:13

cla-bot bot added the cla: yes label Oct 22, 2019

abuiles suggested changes Oct 22, 2019

View reviewed changes

bartekn added 3 commits October 28, 2019 17:38

Check expingest value

30dd4b5

Fix tests

9c4a84b

Merge branch 'release-horizon-v0.23.0' into expingest-account-details

22a04eb

abuiles suggested changes Oct 28, 2019

View reviewed changes

Tests

88bef55

bartekn requested a review from abuiles October 30, 2019 14:50

abuiles suggested changes Oct 30, 2019

View reviewed changes

review

1742a8d

abuiles approved these changes Oct 30, 2019

View reviewed changes

tamirms reviewed Oct 30, 2019

View reviewed changes

tamirms approved these changes Oct 30, 2019

View reviewed changes

bartekn merged commit 233c512 into stellar:release-horizon-v0.23.0 Oct 30, 2019

bartekn deleted the expingest-account-details branch October 30, 2019 19:14

This was referenced Oct 30, 2019

Implement /accounts/{id} endpoint using experimental ingestion #1766

Closed

Compare local state with stellar-core state in actions #1726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

services/horizon: Use new ingestion data in `/accounts/{account_id}` #1868

services/horizon: Use new ingestion data in `/accounts/{account_id}` #1868

bartekn commented Oct 22, 2019

abuiles left a comment •

edited

Loading

bartekn commented Oct 28, 2019

abuiles left a comment

bartekn commented Oct 28, 2019

bartekn commented Oct 30, 2019

abuiles left a comment

abuiles Oct 30, 2019

abuiles Oct 30, 2019

abuiles Oct 30, 2019

bartekn Oct 30, 2019 •

edited

Loading

abuiles Oct 30, 2019

bartekn Oct 30, 2019

bartekn commented Oct 30, 2019

abuiles left a comment

tamirms Oct 30, 2019

bartekn Oct 30, 2019

tamirms Oct 30, 2019

bartekn Oct 30, 2019

services/horizon: Use new ingestion data in /accounts/{account_id} #1868

services/horizon: Use new ingestion data in /accounts/{account_id} #1868

Conversation

bartekn commented Oct 22, 2019

PR Structure

Thoroughness

Release planning

Summary

Known limitations & issues

abuiles left a comment • edited Loading

Choose a reason for hiding this comment

bartekn commented Oct 28, 2019

abuiles left a comment

Choose a reason for hiding this comment

bartekn commented Oct 28, 2019

bartekn commented Oct 30, 2019

abuiles 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

bartekn Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekn commented Oct 30, 2019

abuiles 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

Choose a reason for hiding this comment

services/horizon: Use new ingestion data in `/accounts/{account_id}` #1868

services/horizon: Use new ingestion data in `/accounts/{account_id}` #1868

abuiles left a comment •

edited

Loading

bartekn Oct 30, 2019 •

edited

Loading