Make accounter schedule transactions explicitly #17

mbyczkowski · 2018-10-12T23:29:21Z

This should:
a) simplify backend logic. There are now AddrRequest<->AddrResponses and
TxRequest<->TxResponses pipelines. Backends don't have to track
transactions that are seen in order to avoid sending duplicate
entries to the accounter.
b) make sure accounter stops when all the transactions are received
from the backend. The accounter keeps track of derived and processed
addresses. It can now see how many transactions each address has
and then schedule those transactions accordingly with the backend.
Then it knows, how many (non-duplicate) transactions it has to receive
before completing the fetchTransactions step and moving to
processing all the transaction information.

coveralls · 2018-10-13T01:19:30Z

Pull Request Test Coverage Report for Build 164

44 of 144 (30.56%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.8%) to 38.612%

Changes Missing Coverage	Changed/Added Lines	%
backend/recorder_backend.go	4	0.0%
backend/electrum_backend.go	23	0.0%
backend/btcd_backend.go	73	0.0%

Files with Coverage Reduction	New Missed Lines	%
backend/electrum_backend.go	1	0.0%

Totals
Change from base Build 155:	0.8%
Covered Lines:	434
Relevant Lines:	1124

💛 - Coveralls

accounter/accounter.go

alokmenghrajani · 2018-10-14T14:27:12Z

accounter/accounter.go

+	lastAddresses      [2]uint32
+	derivedAddrCount   uint32
+	processedAddrCount uint32
+	seenTxCount        uint32


we could use a single sync.Wg for all the backend channels here. Increment it before we write to any channel, decrement it when we read data.

Would this give us anything useful? We keep generating more addresses, so at some point WaitGroup can be done, but there might be more data in-flight. Maybe there's an elegant solution I cannot yet see 🤔

alokmenghrajani · 2018-10-14T14:27:58Z

accounter/accounter.go

+			for _, txHash := range resp.TxHashes {
+				if _, exists := a.transactions[txHash]; !exists {
+					a.backend.TxRequest(txHash)
+					a.seenTxCount++


you want to increment a.seenTxCount before making the request?

yes. sloppy coding 😅

alokmenghrajani · 2018-10-14T14:30:04Z

backend/electrum_backend.go

 		eb.txRequests <- txHash
 		return err
 	}
+	height, err := eb.getTxHeight(txHash)
+	if err != nil {
+		// a transaction thas is being asked for is not in the cache, yet (most likely)


Have you figured out why this could happen? For now we can have a panic() here if the cache is inconsistent and I can debug later.

alokmenghrajani · 2018-10-14T14:31:09Z

backend/backend.go

@@ -27,6 +27,7 @@ import (
 type Backend interface {


we'll have to eventually update the comment above this.

you mostly addressed the comment for the Backend interface in other PRs, right?

alokmenghrajani · 2018-10-14T14:34:38Z

backend/backend.go

@@ -27,6 +27,7 @@ import (
 type Backend interface {
 	AddrRequest(addr *deriver.Address)
 	AddrResponses() <-chan *AddrResponse
+	TxRequest(txHash string)
 	TxResponses() <-chan *TxResponse

 	Finish()


Finish() should no longer be needed?

We could get rid off it, but it feels weird to me that we're not closing resources properly. I think backend should have a proper Close() method if someone wanted to use a backend API outside of beancounter.

This should: a) simplify backend logic. There are now AddrRequest<->AddrResponses and TxRequest<->TxResponses pipelines. Backends don't have to track transactions that are seen in order to avoid sending duplicate entries to the accounter. b) make sure accounter stops when all the transactions are received from the backend. The accounter keeps track of derived and processed addresses. It can now see how many transactions each address has and then schedule those transactions accordingly with the backend. Then it knows, how many (non-duplicate) transactions it has to receive before completing the fetchTransactions step and moving to processing all the transaction information.

mbyczkowski force-pushed the simplify-backends branch from 5aa6b9a to f29dfbf Compare October 13, 2018 04:37

alokmenghrajani reviewed Oct 14, 2018

View reviewed changes

accounter/accounter.go Outdated Show resolved Hide resolved

alokmenghrajani reviewed Oct 14, 2018

View reviewed changes

alokmenghrajani approved these changes Oct 14, 2018

View reviewed changes

alokmenghrajani reviewed Oct 14, 2018

View reviewed changes

mbyczkowski force-pushed the simplify-backends branch from f29dfbf to f99aad3 Compare October 16, 2018 06:00

mbyczkowski force-pushed the simplify-backends branch from 959d5a0 to 27a8002 Compare October 16, 2018 19:42

mbyczkowski merged commit 2b1e6a1 into master Oct 16, 2018

mbyczkowski deleted the simplify-backends branch October 16, 2018 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make accounter schedule transactions explicitly #17

Make accounter schedule transactions explicitly #17

mbyczkowski commented Oct 12, 2018

coveralls commented Oct 13, 2018 •

edited

Loading

alokmenghrajani Oct 14, 2018

mbyczkowski Oct 16, 2018

alokmenghrajani Oct 14, 2018

mbyczkowski Oct 16, 2018

alokmenghrajani Oct 14, 2018

alokmenghrajani Oct 14, 2018

mbyczkowski Oct 16, 2018

alokmenghrajani Oct 14, 2018

mbyczkowski Oct 16, 2018

Make accounter schedule transactions explicitly #17

Make accounter schedule transactions explicitly #17

Conversation

mbyczkowski commented Oct 12, 2018

coveralls commented Oct 13, 2018 • edited Loading

Pull Request Test Coverage Report for Build 164

💛 - Coveralls

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

coveralls commented Oct 13, 2018 •

edited

Loading