Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inner Transaction Search #721

Merged
merged 41 commits into from
Oct 26, 2021
Merged

Inner Transaction Search #721

merged 41 commits into from
Oct 26, 2021

Conversation

winder
Copy link
Contributor

@winder winder commented Oct 8, 2021

Summary

Update queries to support returning the root transaction bytes when matching an inner transaction.

Test Plan

New unit tests.

idb/idb.go Show resolved Hide resolved
api/handlers.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2021

Codecov Report

Merging #721 (cb4c7aa) into develop (782b0bc) will increase coverage by 0.78%.
The diff coverage is 79.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #721      +/-   ##
===========================================
+ Coverage    54.09%   54.88%   +0.78%     
===========================================
  Files           23       27       +4     
  Lines         3734     3817      +83     
===========================================
+ Hits          2020     2095      +75     
- Misses        1446     1450       +4     
- Partials       268      272       +4     
Impacted Files Coverage Δ
api/converter_utils.go 86.70% <61.53%> (-0.73%) ⬇️
idb/postgres/postgres.go 47.74% <71.42%> (+1.13%) ⬆️
api/handlers.go 18.18% <81.81%> (+3.14%) ⬆️
idb/idb.go 85.29% <88.00%> (ø)
idb/postgres/internal/writer/writer.go 78.07% <100.00%> (+0.09%) ⬆️
idb/sig_type.go 18.75% <0.00%> (ø)
idb/txn_type_enum.go 60.00% <0.00%> (ø)
idb/idb_factory.go 20.00% <0.00%> (ø)
... and 1 more

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 782b0bc...cb4c7aa. Read the comment docs.

@winder winder self-assigned this Oct 11, 2021
@winder winder linked an issue Oct 11, 2021 that may be closed by this pull request
Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a bit confused. Two questions.

  1. if the filter matches only two inner txns, what needs to be returned?
  2. what does the limit do when inner txns are matched?

idb/idb.go Show resolved Hide resolved
idb/idb.go Outdated Show resolved Hide resolved
idb/idb.go Outdated Show resolved Hide resolved
idb/postgres/postgres.go Show resolved Hide resolved
idb/idb.go Show resolved Hide resolved
api/handlers.go Show resolved Hide resolved
Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides code comments, one problem I see is with paging. Suppose we have these transactions:
0 - root
1 - inner
2 - inner
3 - root

Now I do a query that matches all the transactions with limit 2. The DB layer returns 0 and 1, the API layer return 0 along with next token {round: x, intra: 1, id: ID(0)}. The next query from the user will have limit 1, the DB layer will return txn 2 but the API layer will filter it out and return 0 results. So the user will never get to txn 3!

I suggest, the DB layer will return the new next token, moving the intra past the last inner txn.

idb/postgres/postgres_integration_test.go Outdated Show resolved Hide resolved
api/handlers.go Show resolved Hide resolved
@winder
Copy link
Contributor Author

winder commented Oct 22, 2021

@tolikzinovyev regarding the paging issue... nice catch. Probably not a big deal with this first iteration since there is a maximum of 16 inner transactions, but in the next version there maybe hundreds. I'm going to create a follow-up ticket to focus on that issue.

edit: Created #750

@winder winder mentioned this pull request Oct 22, 2021
@tolikzinovyev
Copy link
Contributor

I'd rather keep the develop branch bug-free and ideally we should change this PR.

I see two options here. Let me know if you think they will not work or there is a better way.

Option 1:
The DB query will not change but the database will return only root txns through TxnRow, performing deduplication. Particularly, TxnRow will be rolled back to the previous version since it already sufficed for root txns. The DB layer will return the new next token as opposed to computing it in the API layer, moving the token's intra past the root txn boundary (either the last inner txn or the root txn's inner depending on which direction we are traversing).

If in the future we need to also return inner txns, we can again extend TxnRow.

Option 2:
We keep the DB layer the way it is here, but API layer will decode the root txn to see how many inner txns it has to compute the new next token correctly.

@winder
Copy link
Contributor Author

winder commented Oct 25, 2021

@tolikzinovyev I was thinking option 2 also. It's really ugly since it requires checking the query flags and figuring out the sort order (for the interface, a postgres implementation detail).

An option 3 could be adding AND txid != ? AND extra->'root-txid' != ?, but I want to avoid changing the query as much as possible.

Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paging seems correct now. Why do we need to update go-algorand?

idb/postgres/postgres_integration_test.go Outdated Show resolved Hide resolved
idb/idb.go Outdated Show resolved Hide resolved
idb/postgres/idb_test.go Outdated Show resolved Hide resolved
idb/postgres/postgres_integration_test.go Outdated Show resolved Hide resolved
idb/postgres/postgres_integration_test.go Outdated Show resolved Hide resolved
api/handlers.go Show resolved Hide resolved
api/handlers.go Show resolved Hide resolved
Copy link
Contributor

@tolikzinovyev tolikzinovyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but please restore the go-algorand version before merging!

@winder winder merged commit 38557a1 into develop Oct 26, 2021
@winder winder deleted the will/search-inner-tx-2 branch June 22, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inner Transaction Search Part 2
7 participants