-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Decrease block time to 3s #3612
base: master
Are you sure you want to change the base?
Conversation
Devcontainer is not working properly, I need it to run tests and adjust Perhaps we need a Hard Fork at engine.SnapshotCache.Add(CreateStorageKey(Prefix_GasPerBlock).AddBigEndian(0u), new StorageItem(5 * GAS.Factor));
// TODO - Add hardfork otherwise storage will be different
// After HARD FORK can you add this HF @shargon ? |
"MillisecondsPerBlock": 15000, | ||
"MaxTransactionsPerBlock": 512, | ||
"MillisecondsPerBlock": 3000, | ||
"MaxTransactionsPerBlock": 256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxTransactionsPerBlock can be kept as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it need to be changed, I think that this comment here contradicts your comment below. I did not understand. You said to cut even more aggressively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to reduce MaxTraceableBlocks
, not MaxTransactionsPerBlock
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all can be reduced, each block wont be able to process that amount of transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# node can handle several thousands of simple NEP-17 transfers per second. It's all about MaxBlockSystemFee
(#3552), not MaxTransactionsPerBlock
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that for a first release these proposed values are good.
They can be increased easily, the opposite is more complicated in my opnion.
So, I think we should just move forward.
@@ -113,8 +113,8 @@ public record ProtocolSettings | |||
StandbyCommittee = Array.Empty<ECPoint>(), | |||
ValidatorsCount = 0, | |||
SeedList = Array.Empty<string>(), | |||
MillisecondsPerBlock = 15000, | |||
MaxTransactionsPerBlock = 512, | |||
MillisecondsPerBlock = 3000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need to change defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better, @roman-khimov , because it may, in the future, reflect in some of our tests that use default. Better to keep consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we will change testnet only before, it's better to avoid default changes
@@ -69,7 +69,7 @@ public record ProtocolSettings | |||
/// <summary> | |||
/// The maximum increment of the <see cref="Transaction.ValidUntilBlock"/> field. | |||
/// </summary> | |||
public uint MaxValidUntilBlockIncrement => 86400000 / MillisecondsPerBlock; | |||
public uint MaxValidUntilBlockIncrement => 86400000 / MillisecondsPerBlock; //TODO keep the same?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is configurable per-network, so I'd rather leave the default as is and configure particular networks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
"MillisecondsPerBlock": 15000, | ||
"MaxTransactionsPerBlock": 5000, | ||
"MillisecondsPerBlock": 3000, | ||
"MaxTransactionsPerBlock": 256, | ||
"MemoryPoolMaxTransactions": 50000, | ||
"MaxTraceableBlocks": 2102400, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that it'd be ~73 days instead of current year. But I'd rather cut it down even more aggressively (see FS network settings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's say 64 or 128?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinions are welcome, your values remind me of E-letter chain. Nothing bad about that, but historic data (#2026) suggests our contracts are more used to somewhat longer history available. NeoFS networks use 17280 (3 days of 15s blocks).
@@ -188,6 +188,9 @@ internal override ContractTask InitializeAsync(ApplicationEngine engine, Hardfor | |||
engine.SnapshotCache.Add(CreateStorageKey(Prefix_Committee), new StorageItem(cachedCommittee)); | |||
engine.SnapshotCache.Add(CreateStorageKey(Prefix_VotersCount), new StorageItem(System.Array.Empty<byte>())); | |||
engine.SnapshotCache.Add(CreateStorageKey(Prefix_GasPerBlock).AddBigEndian(0u), new StorageItem(5 * GAS.Factor)); | |||
// TODO - Add hardfork otherwise storage will be different | |||
// After HARD FORK | |||
engine.SnapshotCache.Add(CreateStorageKey(Prefix_GasPerBlock).AddBigEndian(0u), new StorageItem(5 * GAS.Factor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also should be kept as is, it's a default and it affects block 0 state changes. And practically this doesn't matter, there is some default, it can be tuned for a particular network. Like people regularly run 1s networks with 5 GASPerBlock networks for tests and it's not an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need a hard fork for new default, but maybe not needed and can be kept simple as you said.
// around 2*15/M=30.0/5 ~ 40% block time (for M=5) | ||
ExtendTimerByFactor(2); | ||
// around 4*3/M=12.0/5 ~ 80% block time (for M=5) | ||
ExtendTimerByFactor(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one. In our tests we have not changed any of these parameters and they look scalable by their nature, if we use lower block times we expect lower waiting times in general as well. So I'd not change them unless we have some real proven need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed, I have many tests on that for NeoCompiler.
I had also run in the past with server spread along globe.
I suggest these new parameters.
But if you guys do not want to change we can try with original. But this is my recommended for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roman-khimov, you are right that this method is really tricky....I remember exactly why we created that....And that is really tricky...aheuaheua Luckly we had this idea during the creation of dBFT 2.0, otherwise (other changes would be necessary)......
@@ -27,7 +27,7 @@ namespace Neo.Plugins.StateService.Verification | |||
{ | |||
class VerificationContext | |||
{ | |||
private const uint MaxValidUntilBlockIncrement = 100; | |||
private const uint MaxValidUntilBlockIncrement = 100; // Change to 500!?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa. Unexpected one. It's totally different from the "regular" MVUBI, but 500 here would let more ExtensiblePayloads with signatures float around the network, I doubt the intention here was to have it related to time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should related it with the timeblock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you guys should decide, that is why I left as comment.
Co-authored-by: Christopher Schuchardt <[email protected]>
@@ -193,7 +193,10 @@ internal override ContractTask InitializeAsync(ApplicationEngine engine, Hardfor | |||
engine.SnapshotCache.Add(CreateStorageKey(Prefix_Committee), new StorageItem(cachedCommittee)); | |||
engine.SnapshotCache.Add(_votersCount, new StorageItem(System.Array.Empty<byte>())); | |||
engine.SnapshotCache.Add(CreateStorageKey(Prefix_GasPerBlock).AddBigEndian(0u), new StorageItem(5 * GAS.Factor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @shargon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that maybe it should be enabled with 1 instead of 5 after certain height.
So the hardfork will be triggered by modification the SnapshotCache
withnew StorageItem(5 * GAS.Factor)
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to config.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't change it without affecting post-genesis state. And I see zero reason to do this, mainnet/testnet don't care, this will be adjusted by the committee, private networks are fine with whatever default and they can also adjust it if needed.
Statistics on the mainnet delay: 100ms Slots: 500ms Slots: 1000ms Slots: |
// around 2*15/M=30.0/5 ~ 40% block time (for M=5) | ||
ExtendTimerByFactor(2); | ||
// around 4*3/M=12.0/5 ~ 80% block time (for M=5) | ||
ExtendTimerByFactor(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be computed according the configured time per block
Following a recent request by TC and discussed by Core Devs, here we reduce block time to 3s.
Main modifications:
config
files to 3000msMaxTransactionsPerBlock
to avoid too much load in short time (optional but suggested)ChangeViews
on high latency (optional but suggested)Positive aspects:
Negative aspects: