Skip to content

Conversation

@shargon
Copy link
Member

@shargon shargon commented Nov 21, 2025

Description

Adds a function to recover locked funds after one year, requiring approval from 19 of 21 committee nodes. This recovery can only be executed once the period has elapsed and enforces the signature requirement to ensure security and consensus.

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Unit Testing
  • Run Application
  • Local Computer Tests
  • No Testing

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@shargon shargon requested a review from erikzhang November 21, 2025 11:49
@shargon shargon added the N3 label Nov 21, 2025
@erikzhang
Copy link
Member

Looks good to me. Need #4271

@shargon shargon added the Blocked This issue can't be worked at the moment label Nov 22, 2025
@shargon shargon marked this pull request as ready for review November 25, 2025 11:21
@shargon shargon marked this pull request as draft November 25, 2025 11:22
@shargon shargon removed the Blocked This issue can't be worked at the moment label Nov 25, 2025
@shargon shargon marked this pull request as ready for review November 25, 2025 11:30
@erikzhang
Copy link
Member

Regarding waiting time, please vote. Six months = 👍, one year = ❤️, two years = 🎉.

@shargon
Copy link
Member Author

shargon commented Nov 27, 2025

Regarding waiting time, please vote. Six months = 👍, one year = ❤️, two years = 🎉.

Can one year be equivalent to the MaxTraceableBlocks? It's not the same, but it seems reasonable.

@erikzhang
Copy link
Member

Can one year be equivalent to the MaxTraceableBlocks? It's not the same, but it seems reasonable.

I think they are two different things.

@superboyiii
Copy link
Member

Now we have new exception:
60e23ef99d1ff840ef32bce602945463

Comment on lines 723 to 744
try
{
engine.CallContract(contractHash, "transfer", CallFlags.All,
new VM.Types.Array(engine.ReferenceCounter,
[account.ToArray(), NativeContract.Treasury.Hash.ToArray(), balance, StackItem.Null]));

// Execute transfer
context = engine.CurrentContext;
while (engine.InvocationStack.Contains(context!)) debugger.StepInto();

// check result
var result = engine.Pop().GetBoolean();
if (!result)
throw new InvalidOperationException($"Transfer of {balance} from {account} to {committeeMultiSigAddr} failed in contract {contractHash}.");
}
catch { throw; }
finally
{
// Reset witnesses

state.NativeCallingScriptHash = bak;
}
Copy link
Member

@superboyiii superboyiii Dec 17, 2025

Choose a reason for hiding this comment

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

The issue is: In RecoverFundsFinish, when calling transfer via CallContract, the new execution context wasn’t given NativeCallingScriptHash.

Suggested change
try
{
engine.CallContract(contractHash, "transfer", CallFlags.All,
new VM.Types.Array(engine.ReferenceCounter,
[account.ToArray(), NativeContract.Treasury.Hash.ToArray(), balance, StackItem.Null]));
// Execute transfer
context = engine.CurrentContext;
while (engine.InvocationStack.Contains(context!)) debugger.StepInto();
// check result
var result = engine.Pop().GetBoolean();
if (!result)
throw new InvalidOperationException($"Transfer of {balance} from {account} to {committeeMultiSigAddr} failed in contract {contractHash}.");
}
catch { throw; }
finally
{
// Reset witnesses
state.NativeCallingScriptHash = bak;
}
ExecutionContext? transferContext = null;
UInt160? transferContextBak = null;
try
{
engine.CallContract(contractHash, "transfer", CallFlags.All,
new VM.Types.Array(engine.ReferenceCounter,
[account.ToArray(), NativeContract.Treasury.Hash.ToArray(), balance, StackItem.Null]));
// Set NativeCallingScriptHash for the new context to allow transfer to pass witness check
transferContext = engine.CurrentContext;
var transferContextState = transferContext.GetState<ExecutionContextState>();
transferContextBak = transferContextState.NativeCallingScriptHash;
transferContextState.NativeCallingScriptHash = account;
// Execute transfer
while (engine.InvocationStack.Contains(transferContext!)) debugger.StepInto();
// check result
var result = engine.Pop().GetBoolean();
if (!result)
throw new InvalidOperationException($"Transfer of {balance} from {account} to {NativeContract.Treasury.Hash} failed in contract {contractHash}.");
}
catch { throw; }
finally
{
// Reset witnesses
if (transferContext != null)
{
var transferContextState = transferContext.GetState<ExecutionContextState>();
transferContextState.NativeCallingScriptHash = transferContextBak;
}
state.NativeCallingScriptHash = bak;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try with @roman-khimov idea

Comment on lines 606 to 609
// Remove request funds if any

key = CreateStorageKey(Prefix_BlockedAccountRequestFunds, account);
engine.SnapshotCache.Delete(key);
Copy link
Member

Choose a reason for hiding this comment

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

Does it need an event?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, it would also be acceptable if there were events for blocking and unblocking accounts.

Copy link
Member Author

Choose a reason for hiding this comment

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

}

[ContractMethod(Hardfork.HF_Faun, CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States | CallFlags.AllowNotify)]
public void RecoverFundsFinish(ApplicationEngine engine, UInt160 account, VM.Types.Array extraTokens)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can transfer tokens one by one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we can't reset

Copy link
Member

Choose a reason for hiding this comment

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

Now we can transfer tokens one by one.

if (balance > 0)
{
// transfer
var result = await engine.CallFromNativeContractAsync<bool>(account, contractHash, "transfer",
Copy link
Member Author

@shargon shargon Dec 17, 2025

Choose a reason for hiding this comment

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

@superboyiii I receive an error in your UT in CalculateBonus:

var expectEnd = Ledger.CurrentIndex(snapshot) + 1;
if (expectEnd != end) throw new ArgumentOutOfRangeException(nameof(end));
if (state.BalanceHeight >= end) return BigInteger.Zero;

But it should work with real environment

Copy link
Member Author

Choose a reason for hiding this comment

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

Require #4388

@shargon shargon mentioned this pull request Dec 18, 2025
18 tasks
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.

7 participants