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

Revert "Refuel (#2444)" #2560

Merged
merged 2 commits into from
Aug 5, 2021
Merged

Revert "Refuel (#2444)" #2560

merged 2 commits into from
Aug 5, 2021

Conversation

erikzhang
Copy link
Member

This reverts commit 4d45a79.

@erikzhang
Revert "Refuel (#2444)"
e9fe349 
This reverts commit 4d45a79.
@shargon
Copy link
Member

shargon commented Aug 4, 2021

Check balance before refuel?

@roman-khimov
Copy link
Contributor

I don't see how it could be fixed, so we need to drop it.

At the same time it's an exported method, it's declared in the manifest, saved in the ContractManagement, and we have stateroot computed including it. Therefore to properly remove it now we need to update GAS contract using NativeUpdateHistory.

@shargon
Copy link
Member

shargon commented Aug 4, 2021

@roman-khimov Limit to 1 of gas? Or a small value?

@roman-khimov
Copy link
Contributor

@roman-khimov Limit to 1 of gas? Or a small value?

ABORT

@shargon
Copy link
Member

shargon commented Aug 4, 2021

Or if you used refuel with an address, you can't make a transfer with this address during the same invocation.

@superboyiii
Copy link
Member

superboyiii commented Aug 4, 2021 •
edited
Loading

I don't see how it could be fixed, so we need to drop it.

At the same time it's an exported method, it's declared in the manifest, saved in the ContractManagement, and we have stateroot computed including it. Therefore to properly remove it now we need to update GAS contract using NativeUpdateHistory.

How about resync? Since no contract is using refuel now, I think resync is more easier.

@superboyiii
Copy link
Member

Or if you used refuel with an address, you can't make a transfer with this address during the same invocation.

It's better to revert at this time and find a better way later.

@roman-khimov
Copy link
Contributor

How about resync?

How about Poly?

@superboyiii
Copy link
Member

superboyiii commented Aug 4, 2021 •
edited
Loading

How about resync?

How about Poly?

It only verified the latest state root sent by relayer. Poly doesn't store it.

@shargon
Copy link
Member

shargon commented Aug 4, 2021

It's better to revert at this time and find a better way later.

I think that ABORT it's easier, and we can try to find a solution later

@ZhangTao1596
Copy link

ZhangTao1596 commented Aug 4, 2021 •
edited
Loading

@roman-khimov Limit to 1 of gas? Or a small value?

ABORT

May I ask what ABORT means ? delete the method ?

@shargon
Copy link
Member

shargon commented Aug 4, 2021 •
edited
Loading

May I ask what ABORT means ? delete the method ?

throw an exception during the calling

@superboyiii
Copy link
Member

May I ask what ABORT means ? delete the method ?

throw an exception during the calling

Then we have to tell user invoking Refuel retrun ABORT, it's odd. Delete it makes more sense.

@erikzhang
Copy link
Member Author

Merge?

roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Aug 5, 2021
@roman-khimov
native: drop Refuel method from GAS
603822e 
It can be used to attack the network (amplifying DOS), so it's broken
beyond repair. See also neo-project/neo#2560 and neo-project/neo#2561.
@roman-khimov roman-khimov mentioned this pull request Aug 5, 2021
Merged
roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Aug 5, 2021
@roman-khimov
native: drop Refuel method from GAS
d6bd6b6 
It can be used to attack the network (amplifying DOS), so it's broken
beyond repair. This reverts ac60160.

See also neo-project/neo#2560 and neo-project/neo#2561.
@erikzhang erikzhang merged commit 75ab854 into master Aug 5, 2021
@erikzhang erikzhang deleted the remove-refuel branch August 5, 2021 08:08
@ixje ixje mentioned this pull request Aug 6, 2021
Merged
@roman-khimov roman-khimov mentioned this pull request Aug 17, 2021
Open
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Sep 13, 2021
@ixje
revert refuel (neo #2444) neo-project/neo#2560 (#122)
f16b399
@roman-khimov roman-khimov mentioned this pull request Sep 16, 2023
Closed
5 tasks
@vang1ong7ang vang1ong7ang mentioned this pull request Oct 10, 2023
Merged
@roman-khimov roman-khimov mentioned this pull request Nov 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erikzhang @shargon @roman-khimov @superboyiii @ZhangTao1596