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

Lower MaxValidUntilBlockIncrement limit #2026

Closed
roman-khimov opened this issue Oct 27, 2020 · 10 comments
Closed

Lower MaxValidUntilBlockIncrement limit #2026

roman-khimov opened this issue Oct 27, 2020 · 10 comments
Labels
discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
MaxValidUntilBlockIncrement constant introduced in #765 makes old blocks/transactions unavailable for current running scripts and allows to drop old blocks/transactions from the local storage. It's a very good thing, but at the same time the constant is set at the moment for 2102400 which is a year long life for 15-seconds block interval. I think it's a bit more than needed and we can be more aggressive here, dropping even more data.

We've analyzed Neo 2 mainnet (up to 6286643) and testnet (up to 4935922) chains for their accesses to blocks/headers/transactions (GetBlock/GetHeader/GetTransaction calls). There are 1010043 calls on mainnet and 680920 on testnet and there is a quite expected distribution of relative heights for these accesses:

mainnet:
 720648 0
   5688 1
   3099 2
   4827 3
   3787 4
   2795 5
   2033 6
   2198 7
   1245 8
   1101 9
    975 10
    841 11
    769 12
    657 13
    851 14
    529 15
...

testnet:
 666313 0
    393 1
    368 2
    543 3
    449 4
    315 5
    222 6
    270 7
    125 8
    135 9
    121 10
    121 11
     71 12
     81 13
     90 14
     79 15
...

But of course the list is long for both networks and the most interesting thing here is the length of its tail. And it looks like this:

mainnet:
...
      1 6286580
      2 6286581
      2 6286583
      1 6286584
      1 6286586
      2 6286587
      1 6286591
      1 6286593
      2 6286596
      1 6286603
      1 6286604
      2 6286611
      1 6286613
      1 6286636

testnet:
...
      1 3424281
      2 3424376
      2 3449319
      5 3463905
      6 3463981
      5 3464008
      6 3464018
      1 3464083
      1 3469516
      1 3711048
      1 3767421
      1 3882221
      1 4526755

What was discovered is that blocks 0 and 1 are actually quite popular, so they're responsible for the tail here, there are 16367 such references on mainnet and 4087 on testnet. For those curios wondering what data from blocks 0 and 1 is being used we actually have an answer too (for both chains combined):

     36 "method": "Neo.Block.GetTransaction"
     37 "method": "Neo.Header.GetConsensusData"
     62 "method": "Neo.Header.GetHash"
  20356 "method": "Neo.Header.GetTimestamp"
      6 "method": "Neo.Transaction.GetAttributes"
    155 "method": "Neo.Transaction.GetHash"
      3 "method": "Neo.Transaction.GetInputs"
      9 "method": "Neo.Transaction.GetOutputs"
     10 "method": "Neo.Transaction.GetReferences"

All of these references to 0 and 1 are broken by design on Neo 3 (they'll break at MaxValidUntilBlockIncrement+1), so we can ignore them and if we're to do that, we get the following tails:

mainnet:
...
      1 2452931
      1 2455132
      1 2496815
      1 2498725
      1 2504172
      1 2513515
      1 2553586
      1 2558091
      1 2593907
      1 2631380
      1 2631469
      1 2652178
      1 2890035
      1 3044690

testnet:
...
      1 772506
      2 948099
      2 948100
      1 948103
      1 948104
      1 1657352
      1 1677623
      1 2347118
      1 2354643
      1 2355609
      1 2358376
      1 2694665

Just 5 (out of 676833) calls would fail on testnet and 41 (out of 993676) with the current MaxValidUntilBlockIncrement. But I have to notice that these numbers are not zero already.

But then again, what's more interesting is how this old data is being used, so if we're to concentrate on requests past 128000 blocks from the current that would be 2671 calls for mainnet and 54 for testnet. They do the following requests for these headers/blocks/transactions (combined for mainnet/tesnet):

     14 "method": "Neo.Block.GetTransactions"
   3877 "method": "Neo.Header.GetTimestamp"
    211 "method": "Neo.Transaction.GetHash"
    548 "method": "Neo.Transaction.GetOutputs"
    546 "method": "Neo.Transaction.GetUnspentCoins"

Obviously outputs and unspents are no longer relevant for Neo 3 and GetHash is probably related to those with GetTransactions being a prologue to a deeper dive into transactions. The only thing left is GetTimestamp which tends to be used quite a lot, but 3852 out of 3877 calls to it are actually generated by a single 4c7cca112a8c5666bce5da373010fc0920d0e0d2 mainnet contract and it really has a lot of GetTimestamp calls inside. IIUC it's because of code like this:

            var selling = getAuctionStateByAuctionID(auctionID);
...
                var starttime = Blockchain.GetHeader((uint)selling.startBlockSelling).Timestamp;

But the contract should've just saved the timestamp previously, while storing this selling variable (otherwise it's broken on Neo 3 anyway).

So even though there is a number of 128K+ calls, they're either related to UTXO model or should've been avoided in the first place. Which leads us to the proposal for more aggressive tail cutting on Neo 3.

Do you have any solution you want to propose?
I think we can safely limit MaxValidUntilBlockIncrement to 200K, that would affect just 43 calls from Neo 2 testnet (0.006%) and 2310 from Neo 2 mainnet (0.2%). 200K 15 seconds blocks is just a little more than a month of blockchain's life which I think is practical enough for any contracts, if they need some data from old blocks/transactions they have a month to process it or save it into their storage.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Ledger
  • Network Policy
@erikzhang
Copy link
Member

I think transactions should be kept for at least one year.

@roman-khimov
Copy link
Contributor Author

But what's the rationale for this?

We expect a lot of transactions on NeoFS chain and we're absolutely sure we can trim them more aggressively, but we want to keep the protocol the same, that's why we started analyzing historic data for public chains. And I think we have data here proving that one month is more than enough for public chains too, older data is not really used even on Neo 2.

@Qiao-Jin
Copy link
Contributor

I don't see any benefit for this changing.

@erikzhang
Copy link
Member

erikzhang commented Oct 29, 2020

We expect a lot of transactions on NeoFS chain and we're absolutely sure we can trim them more aggressively

We can move this value to protocol.json. Then NeoFS can set a different value.

@roman-khimov
Copy link
Contributor Author

Yeah, I had this thought too, if it is to be configurable we can use different settings for different networks, let's at least do that for now. Though I still think lower MaxTraceableBlocks is possible and beneficial for public networks.

@roman-khimov
Copy link
Contributor Author

This also has relation to P2P node state synchronization (snapshots or something similar), BTW. As we do plan for a lot of transactions we also want new nodes to synchronize faster than by processing the whole chain, so we plan to do some state synchronization in the future. Of course the node would still also need to have at least MaxTraceableBlocks number of blocks and the lower this value is the simpler it would be.

@Tommo-L
Copy link
Contributor

Tommo-L commented Oct 29, 2020

@roman-khimov Don't forget other parameters that NeoFS may need. We may put them in the configuration file, too.

@Tommo-L
Copy link
Contributor

Tommo-L commented Nov 25, 2020

Close?

@roman-khimov
Copy link
Contributor Author

It's configurable now, so while this still has some data justifying lower MaxTraceableBlocks for testnet/mainnet, we can always get back to it later.

@roman-khimov
Copy link
Contributor Author

Just FYI, on the effects of lowering the MaxTraceableBlocks setting:
https://neospcc.medium.com/cutting-blockchain-tail-with-neogo-5256a120f6bb

Public networks can still benefit from it.

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

Successfully merging a pull request may close this issue.

4 participants