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

Decrease block time to 3s #3612

Open
wants to merge 13 commits into
base: HF_Echidna
Choose a base branch
from
Open

Decrease block time to 3s #3612

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b5fd4ec
First commit and no format
vncoelho Dec 5, 2024
91c4bd2
Start adjusting param but devcontainer not working
vncoelho Dec 5, 2024
9ba85d8
Merge branch 'master' into decreaseblocktime-3s
vncoelho Dec 6, 2024
a095496
Merge branch 'master' into decreaseblocktime-3s
Jim8y Dec 6, 2024
3329c54
Update docs/config.json.md
vncoelho Dec 7, 2024
774471a
Merge branch 'master' into decreaseblocktime-3s
vncoelho Dec 9, 2024
739beaf
format
vncoelho Dec 9, 2024
af42801
Merge branch 'master' into decreaseblocktime-3s
vncoelho Dec 10, 2024
9d677c5
Merge branch 'master' into decreaseblocktime-3s
shargon Dec 12, 2024
cd91440
Update src/Neo.CLI/config.fs.mainnet.json
shargon Dec 12, 2024
26b1e58
Update src/Neo.CLI/config.json
shargon Dec 12, 2024
3b52a3c
Update src/Neo.CLI/config.mainnet.json
shargon Dec 12, 2024
793300c
Merge branch 'master' into decreaseblocktime-3s
Jim8y Dec 17, 2024
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
4 changes: 2 additions & 2 deletions benchmarks/Neo.Benchmarks/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
"ProtocolConfiguration": {
"Network": 860833102,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
"MaxTransactionsPerBlock": 256,
Copy link
Contributor

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.

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 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.

Copy link
Member

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.

Copy link
Contributor

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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually increase the tps for neo, so 256 is acceptable.

"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 2102400,
"Hardforks": {
Expand Down
4 changes: 2 additions & 2 deletions docs/config.json.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ This README provides an explanation for each field in the JSON configuration fil
- **AddressVersion**: Version byte used in Neo address generation. Default is `53`.

### MillisecondsPerBlock
- **MillisecondsPerBlock**: Time interval between blocks in milliseconds. Default is `15000` (15 seconds).
- **MillisecondsPerBlock**: Time interval between blocks in milliseconds. Default is `3600` (3 seconds).
vncoelho marked this conversation as resolved.
Show resolved Hide resolved

### MaxTransactionsPerBlock
- **MaxTransactionsPerBlock**: Maximum number of transactions allowed per block. Default is `512`.
- **MaxTransactionsPerBlock**: Maximum number of transactions allowed per block. Default is `256`.

### MemoryPoolMaxTransactions
- **MemoryPoolMaxTransactions**: Maximum number of transactions that can be held in the memory pool. Default is `50000`.
Expand Down
4 changes: 2 additions & 2 deletions src/Neo.CLI/config.fs.mainnet.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"ProtocolConfiguration": {
"Network": 91414437,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
shargon marked this conversation as resolved.
Show resolved Hide resolved
"MaxTransactionsPerBlock": 256,
"MemoryPoolMaxTransactions": 50000,
shargon marked this conversation as resolved.
Show resolved Hide resolved
"MaxTraceableBlocks": 17280,
"InitialGasDistribution": 5200000000000000,
Expand Down
4 changes: 2 additions & 2 deletions src/Neo.CLI/config.fs.testnet.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"ProtocolConfiguration": {
"Network": 91466898,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
"MaxTransactionsPerBlock": 256,
"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 17280,
"InitialGasDistribution": 5200000000000000,
Expand Down
4 changes: 2 additions & 2 deletions src/Neo.CLI/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"ProtocolConfiguration": {
"Network": 860833102,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
shargon marked this conversation as resolved.
Show resolved Hide resolved
"MaxTransactionsPerBlock": 256,
shargon marked this conversation as resolved.
Show resolved Hide resolved
"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 2102400,
"Hardforks": {
Expand Down
4 changes: 2 additions & 2 deletions src/Neo.CLI/config.mainnet.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"ProtocolConfiguration": {
"Network": 860833102,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
"MaxTransactionsPerBlock": 256,
shargon marked this conversation as resolved.
Show resolved Hide resolved
"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 2102400,
"Hardforks": {
Expand Down
4 changes: 2 additions & 2 deletions src/Neo.CLI/config.testnet.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"ProtocolConfiguration": {
"Network": 894710606,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 5000,
"MillisecondsPerBlock": 3000,
"MaxTransactionsPerBlock": 256,
"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 2102400,
Copy link
Contributor

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).

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 agree, let's say 64 or 128?

Copy link
Contributor

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).

Copy link
Member

@AnnaShaleva AnnaShaleva Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just for the record, I have analyzed the blocks up to 6507174 height of N3 mainnet and checked all calls of native Ledger contract that affected by MaxTraceableBlocks setting (getBlock, getTransaction, getTransactionHeight, getTransactionFromBlock, getTransactionSigners, getTransactionVMState). Turns out that:

  1. There's only 5262 call to these APIs that successfully retrieve block and check if it's traceable. It's much lower than I expected.
  2. Contracts try to reach only very recent blocks, they don't go far than 11 blocks ago. In fact, most calls are trying to reach block accepted 1-5 blocks ago.
  3. Here's some statistics on calls:
PersistingBlockIndex - TargetBlockIndex The number of occurrences
1 921
2 3706
3 606
4 21
5 4
6 0
7 1
8 1
9 0
10 0
11 2
  1. Just in case, here's the log I gathered to track mainnet calls: traceable.log

So from what I see, from the real mainnet contracts' PoW we don't even need 73 days of traceable blocks. In fact, for existing contracts we're good with 1 day. But it's too harsh, and there are other parts of the node that depends on MaxTraceableBlocks setting (like related RPC calls, conflicting transactions tracking, tails cutting functionality and etc.). So I think that 3-7 days will be OK for mainnet.

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 think that, for now, let's keep the same parameter because, automatically, it will reduce proportionally.
So, from 73 days we go to 15.

Later we change in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a year currently, that's the problem. It'd be 73 days, but that's too much also. Our headers are ~700 bytes, so 2M of headers is ~1.4 GB of data. If there is 1 transaction (~250 bytes) in a block that's +0.5 GB immediately. And no one needs this data (other than pure archival purposes).

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 see, I think it is a good reduce and it is not the purpose of this PR.
Maybe keep the current value will be adequated for now because it will also improve.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's have #3644 for this. 3s consensus can have any MTB technically.

"Hardforks": {
Expand Down
6 changes: 3 additions & 3 deletions src/Neo/ProtocolSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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??
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok


/// <summary>
/// Indicates the maximum number of transactions that can be contained in a block.
Expand Down Expand Up @@ -113,8 +113,8 @@ public record ProtocolSettings
StandbyCommittee = Array.Empty<ECPoint>(),
ValidatorsCount = 0,
SeedList = Array.Empty<string>(),
MillisecondsPerBlock = 15000,
MaxTransactionsPerBlock = 512,
MillisecondsPerBlock = 3000,
Copy link
Contributor

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.

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 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.

Copy link
Member

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

MaxTransactionsPerBlock = 256,
MemoryPoolMaxTransactions = 50_000,
MaxTraceableBlocks = 2_102_400,
InitialGasDistribution = 52_000_000_00000000,
Expand Down
3 changes: 3 additions & 0 deletions src/Neo/SmartContract/Native/NeoToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member Author

Choose a reason for hiding this comment

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

ping @shargon

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 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);

Copy link
Member

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?

Copy link
Contributor

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.

// TODO - Add hardfork otherwise storage will be different
// After HARD FORK
engine.SnapshotCache.Add(CreateStorageKey(Prefix_GasPerBlock).AddBigEndian(0u), new StorageItem(5 * GAS.Factor));
Copy link
Contributor

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.

Copy link
Member Author

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.

engine.SnapshotCache.Add(CreateStorageKey(Prefix_RegisterPrice), new StorageItem(1000 * GAS.Factor));
return Mint(engine, Contract.GetBFTAddress(engine.ProtocolSettings.StandbyValidators), TotalAmount, false);
}
Expand Down
4 changes: 2 additions & 2 deletions src/Plugins/DBFTPlugin/Consensus/ConsensusService.Check.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ private bool CheckPrepareResponse()
}

// Timeout extension due to prepare response sent
// around 2*15/M=30.0/5 ~ 40% block time (for M=5)
ExtendTimerByFactor(2);
vncoelho marked this conversation as resolved.
Show resolved Hide resolved
// around 4*3/M=12.0/5 ~ 80% block time (for M=5)
ExtendTimerByFactor(4);
Copy link
Contributor

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.

Copy link
Member Author

@vncoelho vncoelho Dec 6, 2024 •
edited
Loading

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.

Copy link
Member Author

@vncoelho vncoelho Dec 6, 2024 •
edited
Loading

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)......


Log($"Sending {nameof(PrepareResponse)}");
localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakePrepareResponse() });
Expand Down
12 changes: 6 additions & 6 deletions src/Plugins/DBFTPlugin/Consensus/ConsensusService.OnMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ private void OnPrepareRequestReceived(ExtensiblePayload payload, PrepareRequest
}

// Timeout extension: prepare request has been received with success
// 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);
Copy link
Member

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


context.Block.Header.Timestamp = message.Timestamp;
context.Block.Header.Nonce = message.Nonce;
Expand Down Expand Up @@ -171,8 +171,8 @@ private void OnPrepareResponseReceived(ExtensiblePayload payload, PrepareRespons
return;

// Timeout extension: prepare response has been received with success
// 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);

Log($"{nameof(OnPrepareResponseReceived)}: height={message.BlockIndex} view={message.ViewNumber} index={message.ValidatorIndex}");
context.PreparationPayloads[message.ValidatorIndex] = payload;
Expand Down Expand Up @@ -210,8 +210,8 @@ private void OnCommitReceived(ExtensiblePayload payload, Commit commit)
if (commit.ViewNumber == context.ViewNumber)
{
// Timeout extension: commit has been received with success
// around 4*15s/M=60.0s/5=12.0s ~ 80% block time (for M=5)
ExtendTimerByFactor(4);
// around 6*3s/M=18.0s/5=12.0s ~ 120% block time (for M=5)
ExtendTimerByFactor(6);

Log($"{nameof(OnCommitReceived)}: height={commit.BlockIndex} view={commit.ViewNumber} index={commit.ValidatorIndex} nc={context.CountCommitted} nf={context.CountFailed}");

Expand Down
2 changes: 1 addition & 1 deletion src/Plugins/StateService/Verification/VerificationContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ namespace Neo.Plugins.StateService.Verification
{
class VerificationContext
{
private const uint MaxValidUntilBlockIncrement = 100;
private const uint MaxValidUntilBlockIncrement = 100; // Change to 500!??
Copy link
Contributor

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.

Copy link
Member

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

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 think you guys should decide, that is why I left as comment.

private StateRoot root;
private ExtensiblePayload rootPayload;
private ExtensiblePayload votePayload;
Expand Down
2 changes: 1 addition & 1 deletion tests/Neo.Network.RPC.Tests/RpcTestCases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3148,7 +3148,7 @@
"protocol": {
"network": 0,
"validatorscount": 0,
"msperblock": 15000,
"msperblock": 3000,
"maxvaliduntilblockincrement": 1,
"maxtraceableblocks": 1,
"addressversion": 0,
Expand Down
4 changes: 2 additions & 2 deletions tests/Neo.Plugins.OracleService.Tests/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
"ProtocolConfiguration": {
"Network": 5195086,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
"MaxTransactionsPerBlock": 256,
"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 2102400,
"Hardforks": {
Expand Down
2 changes: 1 addition & 1 deletion tests/Neo.UnitTests/SmartContract/Native/UT_NeoToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ public void TestEconomicParameter()

(BigInteger, bool) result = Check_GetGasPerBlock(clonedCache, persistingBlock);
result.Item2.Should().BeTrue();
result.Item1.Should().Be(5 * NativeContract.GAS.Factor);
result.Item1.Should().Be(5 * NativeContract.GAS.Factor); // Test should occur with all hardfork enable TODO

persistingBlock = new Block { Header = new Header { Index = 10 } };
(VM.Types.Boolean, bool) result1 = Check_SetGasPerBlock(clonedCache, 10 * NativeContract.GAS.Factor, persistingBlock);
Expand Down
6 changes: 3 additions & 3 deletions tests/Neo.UnitTests/UT_ProtocolSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public void TestGetMemoryPoolMaxTransactions()
[TestMethod]
public void TestGetMillisecondsPerBlock()
{
TestProtocolSettings.Default.MillisecondsPerBlock.Should().Be(15000);
TestProtocolSettings.Default.MillisecondsPerBlock.Should().Be(3000);
}

[TestMethod]
Expand Down Expand Up @@ -131,8 +131,8 @@ internal static string CreateHFSettings(string hf)
""ProtocolConfiguration"": {
""Network"": 860833102,
""AddressVersion"": 53,
""MillisecondsPerBlock"": 15000,
""MaxTransactionsPerBlock"": 512,
""MillisecondsPerBlock"": 3000,
""MaxTransactionsPerBlock"": 256,
""MemoryPoolMaxTransactions"": 50000,
""MaxTraceableBlocks"": 2102400,
""Hardforks"": {
Expand Down
4 changes: 2 additions & 2 deletions tests/Neo.UnitTests/test.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
"ProtocolConfiguration": {
"Network": 860833102,
"AddressVersion": 53,
"MillisecondsPerBlock": 15000,
"MaxTransactionsPerBlock": 512,
"MillisecondsPerBlock": 3000,
"MaxTransactionsPerBlock": 256,
"MemoryPoolMaxTransactions": 50000,
"MaxTraceableBlocks": 2102400,
"Hardforks": {},
Expand Down
Loading