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.

Sign up for GitHub

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

Jump to bottom

Patch: Potential fix for panic on call to block w/ group resource sharing. #1590

Merged
merged 2 commits into from
Dec 31, 2023
Merged

Patch: Potential fix for panic on call to block w/ group resource sharing. #1590

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1008ac
Potential fix for panic on call to block w/ group resource sharing.
gmalouf Dec 31, 2023
50a40d7
Typo fix.
gmalouf Dec 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions api/converter_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,20 +518,16 @@
}
if len(stxn.ApplyData.EvalDelta.LocalDeltas) > 0 {
keys := make([]tuple, 0)

for k := range stxn.ApplyData.EvalDelta.LocalDeltas {
if k == 0 {
keys = append(keys, tuple{
key: 0,
address: stxn.Txn.Sender,
})
} else {
addr := sdk.Address{}
copy(addr[:], stxn.Txn.Accounts[k-1][:])
keys = append(keys, tuple{
key: k,
address: addr,
})
addr, err := edIndexToAddress(k, stxn.Txn, stxn.ApplyData.EvalDelta.SharedAccts)
if err != nil {
return generated.Transaction{}, err

Check warning on line 525 in api/converter_utils.go

View check run for this annotation

Codecov / codecov/patch

api/converter_utils.go#L525 

Added line #L525 was not covered by tests
}
keys = append(keys, tuple{
key: k,
address: addr,
})
}
sort.Slice(keys, func(i, j int) bool { return keys[i].key < keys[j].key })
d := make([]generated.AccountStateDelta, 0)
Expand Down Expand Up @@ -629,6 +625,20 @@
return txn, nil
}

func edIndexToAddress(index uint64, txn sdk.Transaction, shared []sdk.Address) (sdk.Address, error) {
// index into [Sender, txn.Accounts[0], txn.Accounts[1], ..., shared[0], shared[1], ...]
switch {
case index == 0:
return txn.Sender, nil
case int(index-1) < len(txn.Accounts):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this new logic should be conditional based on protocol version, @jannotti ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say it's ok. This is a copy of the same code in go-algorand. My reasoning is that the old would error if the index was out of bounds. This code will too, unless the bug that caused a large index also created a SharedAccts array (which would not exist in old versions). So the behavior should always be the same as it was unless we have two coincidental bugs that happen to compensate for each other. In such a case, we return the account that has somehow made its way into SharedAccts

return txn.Accounts[index-1], nil
case int(index-1)-len(txn.Accounts) < len(shared):
return shared[int(index-1)-len(txn.Accounts)], nil
default:
return sdk.Address{}, fmt.Errorf("invalid Account Index %d in LocalDelta", index)

Check warning on line 638 in api/converter_utils.go

View check run for this annotation

Codecov / codecov/patch

api/converter_utils.go#L635-L638 

Added lines #L635 - L638 were not covered by tests
}
}

func (si *ServerImplementation) assetParamsToAssetQuery(params generated.SearchForAssetsParams) (idb.AssetsQuery, error) {
creator, errorArr := decodeAddress(params.Creator, "creator", make([]string, 0))
if len(errorArr) != 0 {
Expand Down
Loading