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

API: Fix prove bug on API #3632

Merged
merged 3 commits into from
Feb 22, 2022
Merged

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented Feb 15, 2022

Summary

This PR fixes a bug on algod's API. When a tree contains a missing child (not a full tree), the api handler omits this from the proof response and leads to a root mismatch

Test Plan

Add unit tests as well as convert the e2e to test this edge case.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #3632 (d5d2d3e) into master (a6f1b9e) will increase coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3632      +/-   ##
==========================================
+ Coverage   48.11%   48.13%   +0.02%     
==========================================
  Files         381      381              
  Lines       62214    62217       +3     
==========================================
+ Hits        29933    29951      +18     
+ Misses      28848    28837      -11     
+ Partials     3433     3429       -4     
Impacted Files Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
crypto/merklearray/proof.go 100.00% <100.00%> (ø)
cmd/tealdbg/debugger.go 71.42% <0.00%> (-0.99%) ⬇️
data/abi/abi_type.go 88.62% <0.00%> (+0.94%) ⬆️
ledger/acctupdates.go 66.41% <0.00%> (+0.94%) ⬆️
network/wsPeer.go 68.05% <0.00%> (+2.22%) ⬆️

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 a6f1b9e...d5d2d3e. Read the comment docs.

s, _, _ := blk.DecodeSignedTxn(blk.Payset[i])
t.Log(s.Txn.ID())
}
t.FailNow()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the intent here is correct - but you should log more useful information that would help you to debug the issue further.
In particular, I'd suggest dumping the entire confirmedTx ( which includes the block number ), as well as the entire block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the advice

@id-ms id-ms marked this pull request as draft February 15, 2022 13:48
@id-ms id-ms force-pushed the test-txnmerkel branch 6 times, most recently from fcd76d2 to a1f81db Compare February 17, 2022 07:01
@id-ms id-ms changed the title add logging to merkle test Fix merkle prove bug Feb 21, 2022
@id-ms id-ms marked this pull request as ready for review February 21, 2022 10:39
@id-ms id-ms changed the title Fix merkle prove bug Fix merkle prove bug on API Feb 21, 2022
@id-ms id-ms changed the title Fix merkle prove bug on API Merkle Tree: Fix prove bug on API Feb 22, 2022
@tsachiherman tsachiherman changed the title Merkle Tree: Fix prove bug on API API: Fix prove bug on API Feb 22, 2022
@tsachiherman tsachiherman merged commit 88131be into algorand:master Feb 22, 2022
@id-ms id-ms deleted the test-txnmerkel branch February 28, 2022 16:11
This was referenced Feb 28, 2022
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.

3 participants