On API, include logs in block (when ?withLogs=true) #4127

andreibancioiu · 2022-05-28T09:07:32Z

On Block API endpoint, include logs when the query parameter withLogs is set to true.

codecov-commenter · 2022-05-28T20:10:25Z

Codecov Report

❗ No coverage uploaded for pull request base (feat/rosetta@d71c33a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 1af03c3 differs from pull request most recent head e0fbeee. Consider uploading reports for the commit e0fbeee to get more accurate results

@@               Coverage Diff               @@
##             feat/rosetta    #4127   +/-   ##
===============================================
  Coverage                ?   75.28%           
===============================================
  Files                   ?      626           
  Lines                   ?    82605           
  Branches                ?        0           
===============================================
  Hits                    ?    62187           
  Misses                  ?    15706           
  Partials                ?     4712

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d71c33a...e0fbeee. Read the comment docs.

…ith-logs

node/external/blockAPI/baseBlock.go

node/external/blockAPI/baseBlock_test.go

node/external/blockAPI/metaBlock.go

node/external/blockAPI/shardBlock.go

node/external/logs/logsFacade.go

storage/errors.go

storage/pruning/fullHistoryPruningStorer.go

testscommon/genericMocks/chainStorerMock.go

iulianpascalau · 2022-05-30T10:49:53Z

api/groups/blockGroup.go

-	GetBlockByHash(hash string, withTxs bool) (*api.Block, error)
-	GetBlockByNonce(nonce uint64, withTxs bool) (*api.Block, error)
-	GetBlockByRound(round uint64, withTxs bool) (*api.Block, error)
+	GetBlockByHash(hash string, options api.BlockQueryOptions) (*api.Block, error)


iulianpascalau · 2022-05-30T10:53:46Z

facade/nodeFacade.go

@@ -410,18 +411,18 @@ func (nf *nodeFacade) GetThrottlerForEndpoint(endpoint string) (core.Throttler,
 }

 // GetBlockByHash return the block for a given hash
-func (nf *nodeFacade) GetBlockByHash(hash string, withTxs bool) (*apiData.Block, error) {
-	return nf.apiResolver.GetBlockByHash(hash, withTxs)
+func (nf *nodeFacade) GetBlockByHash(hash string, options api.BlockQueryOptions) (*apiData.Block, error) {


you glued the facade with the api package. Please define the BlockQueryOptions here or in the common area.

The package was already dependent on elrond-go-core/data/api. Fixed to use the already defined import alias.

ok, my mistake 👍

api/groups/blockGroup_test.go

node/external/blockAPI/baseBlock.go

iulianpascalau · 2022-05-30T11:18:00Z

node/external/transactionAPI/apiTransactionResults.go

-	tx.Logs = logsAPI
+	tx.Logs, err = arp.logsFacade.GetLog(hash, epoch)
+	if err != nil && !errors.Is(err, storage.ErrKeyNotFound) {
+		log.Warn("putLogsInTransaction()", "hash", hash, "epoch", epoch, "err", err)


it is ok to silent these? Also on L132

If the original error is "key not found in storage", that should normally be tolerated, even ignored (there are transactions without logs).

Indeed, if the original error is something different, we should not tolerate the error.

However, since we've reverted the pruningStorer / fullHistoryPruningStorer to not emit storage.ErrKeyNotFound anymore, it's a bit tricky to reason about the error type in this piece of code.

As a temporary workaround, I've changed to emit a "debug" log entry on all errors (even though, as previously stated, it's should be quite normal for some transactions to not have any events & logs attached).

Is this all right?

iulianpascalau · 2022-05-30T11:19:17Z

node/external/transactionAPI/check.go

@@ -36,6 +36,9 @@ func checkNilArgs(arg *ArgAPITransactionProcessor) error {
 	if check.IfNil(arg.TxTypeHandler) {
 		return process.ErrNilTxTypeHandler
 	}
+	if check.IfNil(arg.LogsFacade) {
+		return ErrNilLogsFacade


The test is located in apiTransactionProcessor_test.go: NilLogsFacade.

Is it all right, as defined using t.Run()?

storage/pruning/fullHistoryPruningStorer.go

storage/pruning/pruningStorer.go

testscommon/genericMocks/chainStorerMock.go

miiu96 · 2022-05-30T11:27:35Z

node/external/transactionAPI/apiTransactionResults.go

@@ -111,23 +116,21 @@ func (arp *apiTransactionResultsProcessor) putSmartContractResultsInTransactionB
 }

 func (arp *apiTransactionResultsProcessor) putLogsInTransaction(hash []byte, tx *transaction.ApiTransactionResult, epoch uint32) {


maybe you can change the function name
this functions load something from storage and set a field in the transactions structure

Renamed to loadLogsIntoTransaction.

miiu96 · 2022-05-30T12:58:37Z

node/external/transactionAPI/apiTransactionResults.go

+	tx.Logs, err = arp.logsFacade.GetLog(hash, epoch)
+	if err != nil && !errors.Is(err, storage.ErrKeyNotFound) {
+		log.Warn("putLogsInTransaction()", "hash", hash, "epoch", epoch, "err", err)
+	}
 }

 func (arp *apiTransactionResultsProcessor) putLogsInSCR(scrHash []byte, epoch uint32, scr *transaction.ApiSmartContractResult) {


also this maybe can be renamed.

Renamed to loadLogsIntoContractResults.

iulianpascalau · 2022-06-02T05:57:51Z

node/external/blockAPI/baseBlock.go

 	}

+	// TODO (important): define a separate proto-structure for "batch of miniblocks from receipts storage".


TODO no longer relevant. As searched through the code it is ok to be as it is. My mistake, again :D

iulianpascalau · 2022-06-02T06:06:59Z

node/external/logs/logsRepository.go

@@ -12,25 +15,25 @@ type logsRepository struct {
 	marshalizer marshal.Marshalizer
 }

-func newLogsRepository(args argsNewLogsRepository) *logsRepository {
-	storer := args.StorageService.GetStorer(dataRetriever.TxLogsUnit)
+func newLogsRepository(storageService dataRetriever.StorageService, marshalizer marshal.Marshalizer) *logsRepository {


could have been renamed to marshaller (we are slowly move away from the "marshalizer" naming)

Renamed (but only in package /logs for now).

iulianpascalau · 2022-06-02T06:08:35Z

node/external/transactionAPI/apiTransactionResults.go


 		tx.SmartContractResults = append(tx.SmartContractResults, scrAPI)
 	}
 }

-func (arp *apiTransactionResultsProcessor) putLogsInTransaction(hash []byte, tx *transaction.ApiTransactionResult, epoch uint32) {
+func (arp *apiTransactionResultsProcessor) loadLogsIntoTransaction(hash []byte, tx *transaction.ApiTransactionResult, epoch uint32) {


iulianpascalau · 2022-06-02T06:17:14Z

node/external/transactionAPI/apiTransactionResults.go

-	if err != nil && !errors.Is(err, storage.ErrKeyNotFound) {
-		log.Warn("putLogsInTransaction()", "hash", hash, "epoch", epoch, "err", err)
+	if err != nil {
+		// TODO: We should unwrap the error, reason about its type and possibly ignore "key not found in storage" errors.


if those errors are to be expected in some cases, we might print them as TRACE and remove TODOs: valid for all occurrences. Can be done in another PR

Removed TODO. Switched to TRACE.

iulianpascalau · 2022-06-02T06:17:56Z

node/external/transactionAPI/check.go

@@ -36,6 +36,9 @@ func checkNilArgs(arg *ArgAPITransactionProcessor) error {
 	if check.IfNil(arg.TxTypeHandler) {
 		return process.ErrNilTxTypeHandler
 	}
+	if check.IfNil(arg.LogsFacade) {
+		return ErrNilLogsFacade


iulianpascalau

forgotten change in node/external/transactionAPI/apiTransactionResults.go L130. Can be done in another PR

andreibancioiu added 7 commits May 26, 2022 16:45

Sketch.

f59e9ef

Add "block query options" (with txs + with logs).

e345e51

Merge branch 'feat/rosetta' into with-logs

be95d55

Merge branch 'api-sort' into with-logs

ccb5895

Sketch implementation (logsRepository, bring logs in block).

32fd67d

Populate tx.Hash (useful in API).

105b6c4

Fix inclusion of logs.

502dae5

andreibancioiu self-assigned this May 28, 2022

andreibancioiu added 3 commits May 28, 2022 14:09

Fix some tests.

c67db86

Rename parameter.

e24211c

Fix remaining tests.

a8adcde

andreibancioiu and others added 9 commits May 28, 2022 23:12

Refactor / rename.

b63d8d4

Refactor, cleanup, simplify.

6d9b440

KeyValuePair moved to core/storage.

f3e3aff

Extra notes (from a previous PR).

2ae5ef1

Merge branch 'feat/rosetta' into with-logs

205c586

Merge branch 'with-logs' of github.com:ElrondNetwork/elrond-go into w…

2a04744

…ith-logs

Fix condition for logging. Wrap errors.

aea0d55

Add unit tests for logsFacade. Improve generic mock.

770aacc

Extra tests.

73ac756

andreibancioiu marked this pull request as ready for review May 29, 2022 22:13

Fix linter issues.

5ce6b8f

bogdan-rosianu self-requested a review May 30, 2022 07:33

bogdan-rosianu added the type:feature New feature or request label May 30, 2022

bogdan-rosianu reviewed May 30, 2022

View reviewed changes

iulianpascalau self-requested a review May 30, 2022 10:49

AdoAdoAdo previously approved these changes May 30, 2022

View reviewed changes

iulianpascalau reviewed May 30, 2022

View reviewed changes

miiu96 reviewed May 30, 2022

View reviewed changes

Fix after review (part 1).

9621024

andreibancioiu dismissed AdoAdoAdo’s stale review via 9621024 May 30, 2022 21:35

andreibancioiu added 4 commits May 31, 2022 22:52

Merge branch 'feat/rosetta' into with-logs

710711c

Fix after review (part 2).

463b476

Fix after review (part 3) - error propagation.

8805d24

Fix after review - part 4.

6ab38cb

andreibancioiu requested review from iulianpascalau, bogdan-rosianu, AdoAdoAdo and miiu96 June 1, 2022 16:32

andreibancioiu added the area:node-api Node API label Jun 1, 2022

iulianpascalau reviewed Jun 2, 2022

View reviewed changes

iulianpascalau previously approved these changes Jun 2, 2022

View reviewed changes

Fix after review.

2400099

andreibancioiu dismissed iulianpascalau’s stale review via 2400099 June 2, 2022 08:01

iulianpascalau previously approved these changes Jun 2, 2022

View reviewed changes

miiu96 previously approved these changes Jun 2, 2022

View reviewed changes

Merge branch 'feat/rosetta' into with-logs

03a122a

bogdan-rosianu previously approved these changes Jun 2, 2022

View reviewed changes

Fix after review.

e0fbeee

andreibancioiu dismissed stale reviews from bogdan-rosianu, miiu96, and iulianpascalau via e0fbeee June 2, 2022 10:08

bogdan-rosianu approved these changes Jun 2, 2022

View reviewed changes

miiu96 approved these changes Jun 2, 2022

View reviewed changes

iulianpascalau approved these changes Jun 2, 2022

View reviewed changes

andreibancioiu merged commit 0e630b2 into feat/rosetta Jun 2, 2022

andreibancioiu deleted the with-logs branch June 2, 2022 10:35

andreibancioiu added complexity:high interested:api-multiversx-com labels Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On API, include logs in block (when ?withLogs=true) #4127

On API, include logs in block (when ?withLogs=true) #4127

andreibancioiu commented May 28, 2022 •

edited

codecov-commenter commented May 28, 2022 •

edited

iulianpascalau May 30, 2022

iulianpascalau May 30, 2022

andreibancioiu May 30, 2022

iulianpascalau Jun 2, 2022

iulianpascalau May 30, 2022

andreibancioiu Jun 1, 2022 •

edited

iulianpascalau May 30, 2022

andreibancioiu Jun 1, 2022

iulianpascalau Jun 2, 2022

miiu96 May 30, 2022

andreibancioiu Jun 1, 2022

miiu96 May 30, 2022

andreibancioiu Jun 1, 2022

iulianpascalau Jun 2, 2022

andreibancioiu Jun 2, 2022

iulianpascalau Jun 2, 2022

andreibancioiu Jun 2, 2022

iulianpascalau Jun 2, 2022

iulianpascalau Jun 2, 2022

andreibancioiu Jun 2, 2022

iulianpascalau Jun 2, 2022

iulianpascalau left a comment

		@@ -111,23 +116,21 @@ func (arp *apiTransactionResultsProcessor) putSmartContractResultsInTransactionB
		}

		func (arp apiTransactionResultsProcessor) putLogsInTransaction(hash []byte, tx transaction.ApiTransactionResult, epoch uint32) {

		}

		// TODO (important): define a separate proto-structure for "batch of miniblocks from receipts storage".

On API, include logs in block (when ?withLogs=true) #4127

On API, include logs in block (when ?withLogs=true) #4127

Conversation

andreibancioiu commented May 28, 2022 • edited

codecov-commenter commented May 28, 2022 • edited

Codecov Report

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

andreibancioiu Jun 1, 2022 • edited

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

Choose a reason for hiding this comment

iulianpascalau left a comment

Choose a reason for hiding this comment

andreibancioiu commented May 28, 2022 •

edited

codecov-commenter commented May 28, 2022 •

edited

andreibancioiu Jun 1, 2022 •

edited