Increased block size up to 4M; txes are allowed to occupy only a subs… #149

alsala · 2021-07-07T13:03:31Z

…et (default half) of the block size while this limit does not apply to certs

i-Alex · 2021-07-08T06:48:06Z

src/miner.cpp

                continue;
            }

            // Prioritise by fee once past the priority size or we run out of high-priority
            // transactions:
            if (!fSortedByFee &&
-                ((nBlockSize + nTxSize >= nBlockPrioritySize) || !AllowFree(dPriority)))
+                ((nBlockSize + nTxBaseSize >= nBlockPrioritySize) || !AllowFree(dPriority)))


It seems that for the SC support block by default we increased the nBlockPrioritySize value from 1Mb to 2Mb, so in case we will have the block with Txs only, it will be possible to create a full BlockTxPartition with free transactions only.

A possible option can be to have different policy for certs and txs in context of priority. For Txs we will allow as before 1 MB of txs partition at most. Considering the fact certs have 2Mb more "space" in the block without Txs, populate block with certs by fee only, but allow zero fee certs as well, for example.

For the time being I will set the default priority size value to half the value of the tx partition size, that would comply with the first consideration.

The second part of the comment will be discussed in the final code review.

We also must be careful when we are sorting cetificates based on fee: if we are run out of priority space (or the miner set it to 0) and a certificate has a feeRate < minFeeRate, the certificate would not be included in the block. Therefore we should also add a check when creating a certificate and broadcasting to the network.

After discussion, we don't need to use the maximum priority for certificates because they already have a mechanism to be prioritized over txs up to 2MB. After having reached that limit they will compete by fee with txs. Remove the specific implementation for certs of CCertificateMemPoolEntry::GetPriority and use the standard way for calculating the priority.

Priority block size will be kept at 1MB even if currently is almost useless (the current code base contains a bug because ComputePriority does not check if the tx contains joinsplits then nothing is prioritized if not specifically set by the miner through api).

i-Alex · 2021-07-08T06:53:39Z

src/main.cpp

+    if (block.nVersion != BLOCK_VERSION_SC_SUPPORT)
+        block_size_limit = MAX_BLOCK_SIZE_BEFORE_SC;
+
+    if (block.vtx.empty() || (block.vtx.size() + block.vcert.size()) > block_size_limit || ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION) > block_size_limit)


is it correct to compare txs + certs number with block size in bytes: (block.vtx.size() + block.vcert.size()) > block_size_limit? Seems redundant.

We agreed that it might as well be removed, it is comparing different sizes (cardinality vs block size)

i-Alex · 2021-07-08T06:55:17Z

src/main.cpp

+        nTxPartSize += tx.GetSerializeSize(SER_NETWORK, PROTOCOL_VERSION);
+    }
+
+    if (block.nVersion == BLOCK_VERSION_SC_SUPPORT && nTxPartSize > BLOCK_TX_PARTITION_SIZE)


We may have 2 different loops: first one with nTxPartSize check only and if there are errors then do another one with CheckTransaction to prevent complex unnecessary verification:

if (block.nVersion == BLOCK_VERSION_SC_SUPPORT) { for(const CTransaction& tx: block.vtx) { nTxPartSize += tx.GetSerializeSize(SER_NETWORK, PROTOCOL_VERSION); if (nTxPartSize > BLOCK_TX_PARTITION_SIZE) return error(...); } } for(const CTransaction& tx: block.vtx) { if (!CheckTransaction(tx, state, verifier)) { return error("CheckBlock(): CheckTransaction failed"); } }

Agree, in case of attack the check in this approach is more efficient

cronicc · 2021-07-19T10:15:41Z

After review I didn't find anything wrong. I'll give approval after I had a chance to test this on air gapped testnet later this week.
I'll also create a separate branch with all PRs including this merged for a CI run.

@alsala A couple of questions came up though:

Do we need to adjust block heights in any RPC tests so that they run after SC fork activation (thus making sure they still pass with the new rules), or has this already been done?
Do we need to adjust any existing tests to incorporate cert tx? Tests that are related to filtering on the block(template), mempool or RPC level come to mind, e.g. getblocktemplate_priority.py, mempool_tx_input_limit.py, listtransactions.py, prioritisetransaction.py.
Related to 2, do -mempooltxinputlimit and -blockmaxcomplexity apply to certificates and should they?

cronicc · 2021-07-19T11:40:36Z

Conflicts in qa/rpc-tests/test_framework/mc_test/mc_test.py with #134

This is blocking further testing at the moment.

alsala · 2021-07-19T12:54:29Z

After review I didn't find anything wrong. I'll give approval after I had a chance to test this on air gapped testnet later this week.
I'll also create a separate branch with all PRs including this merged for a CI run.

@alsala A couple of questions came up though:

1. Do we need to adjust block heights in any RPC tests so that they run after SC fork activation (thus making sure they still pass with the new rules), or has this already been done?

2. Do we need to adjust any existing tests to incorporate cert tx? Tests that are related to filtering on the block(template), mempool or RPC level come to mind, e.g. [getblocktemplate_priority.py](https://github.com/HorizenOfficial/zend_oo/blob/as/increase_block_size/qa/rpc-tests/getblocktemplate_priority.py), [mempool_tx_input_limit.py](https://github.com/HorizenOfficial/zend_oo/blob/as/increase_block_size/qa/rpc-tests/mempool_tx_input_limit.py), [listtransactions.py](https://github.com/HorizenOfficial/zend_oo/blob/as/increase_block_size/qa/rpc-tests/listtransactions.py), [prioritisetransaction.py](https://github.com/HorizenOfficial/zend_oo/blob/as/increase_block_size/qa/rpc-tests/prioritisetransaction.py).

3. Related to 2, do `-mempooltxinputlimit` and `-blockmaxcomplexity` apply to certificates and should they?

I would suggest to open a couple of separate issues for (1) and (2) above. For instance in (1) we could increase the size of the node cache blockchain enlarging it from 200 to 420, which in regtest is the height of the SC fork, and probably some test would be modified accordingly.
As far as (3) is concerned, yes both those parameters are handled for txes and certificates.

cronicc · 2021-07-19T19:06:43Z

Linker error on MinGW/Windows build https://travis-ci.com/github/HorizenOfficial/zend_oo/jobs/525496892#L14296-L14320

This is not necessarily caused by this branch, as the CI run was with most open PRs combined into https://github.com/HorizenOfficial/zend_oo/tree/cr/sidechains_integration_step4_test_merge.

Edit: Not an issue with this PR but a regression from package crate updates: #152

…et (default half) of the block size while this limit does not apply to certs

…and cert size

…on; added a py test

…tor for the case when compressed data are larger than original ones; added UT for bzip2 algo

…for windows (issue #152)

i-Alex · 2021-07-29T15:26:13Z

src/txmempool.h

@@ -18,6 +18,9 @@ class CAutoFile;

 inline double AllowFreeThreshold()
 {
+    // the threshold represents a one day old, 1 ZEN coin (144*4 is the expected number of blocks per day) and a transaction size of 250 bytes.
+    // TODO, check this: should be:
+    // return COIN * 144 * 4 / 250;


qa/rpc-tests/sc_cert_bwt_amount_rounding.py

qa/rpc-tests/sc_block_partitions.py

qa/rpc-tests/sc_cert_bwt_amount_rounding.py

albertog78 · 2021-08-11T16:22:05Z

src/init.cpp

@@ -510,7 +510,10 @@ std::string HelpMessage(HelpMessageMode mode)
    strUsage += HelpMessageGroup(_("Block creation options:"));
    strUsage += HelpMessageOpt("-blockminsize=<n>", strprintf(_("Set minimum block size in bytes (default: %u)"), 0));
    strUsage += HelpMessageOpt("-blockmaxsize=<n>", strprintf(_("Set maximum block size in bytes (default: %d)"), DEFAULT_BLOCK_MAX_SIZE));
-    strUsage += HelpMessageOpt("-blockprioritysize=<n>", strprintf(_("Set maximum size of high-priority/low-fee transactions in bytes (default: %d)"), DEFAULT_BLOCK_PRIORITY_SIZE));
+
+    strUsage += HelpMessageOpt("-blocktxpartitionmaxsize=<n>", strprintf(_("Set maximum partition block size for transcations in bytes (default: %u)"), DEFAULT_BLOCK_TX_PART_MAX_SIZE));


why a user can set it to an arbitrary value?

Ok, it is now applicable only in regtest

src/main.cpp

albertog78 · 2021-08-11T16:27:47Z

src/main.cpp

+    if (block.nVersion != BLOCK_VERSION_SC_SUPPORT)
+        block_size_limit = MAX_BLOCK_SIZE_BEFORE_SC;
+
+    if (block.vtx.empty() || ::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION) > block_size_limit)


In this way we are serializing all txs twice. Is it possible to customize a method to perform both checks for whole block and txs?

Done, added a new CBlock method for getting block size and total tx size

src/main.cpp

alsala · 2021-08-27T10:22:32Z

src/miner.cpp

                continue;
            }

            // Prioritise by fee once past the priority size or we run out of high-priority
            // transactions:
            if (!fSortedByFee &&
-                ((nBlockSize + nTxSize >= nBlockPrioritySize) || !AllowFree(dPriority)))
+                ((nBlockSize + nTxBaseSize >= nBlockPrioritySize) || !AllowFree(dPriority)))


After discussion, we don't need to use the maximum priority for certificates because they already have a mechanism to be prioritized over txs up to 2MB. After having reached that limit they will compete by fee with txs. Remove the specific implementation for certs of CCertificateMemPoolEntry::GetPriority and use the standard way for calculating the priority.

Priority block size will be kept at 1MB even if currently is almost useless (the current code base contains a bug because ComputePriority does not check if the tx contains joinsplits then nothing is prioritized if not specifically set by the miner through api).

alsala · 2021-08-27T10:26:10Z

src/main.cpp

@@ -1303,8 +1303,12 @@ MempoolReturnValue AcceptCertificateToMemoryPool(CTxMemPool& pool, CValidationSt
        CCertificateMemPoolEntry entry(cert, nFees, GetTime(), dPriority, chainActive.Height());
        unsigned int nSize = entry.GetCertificateSize();

+        unsigned int block_priority_size = DEFAULT_BLOCK_PRIORITY_SIZE;
+        if (!ForkManager::getInstance().areSidechainsSupported(nextBlockHeight))


Remove it. Useless if

…ock_size

…ax priority

alsala requested review from i-Alex and ptagl July 7, 2021 13:03

i-Alex suggested changes Jul 8, 2021

View reviewed changes

alsala requested a review from cronicc July 16, 2021 14:16

This was referenced Jul 21, 2021

adjust block heights in RPC tests, where appropriate, so that they run after SC fork activation #150

Open

adjust existing RPC tests, where appropriate, to incorporate certificates #151

Open

alsala added 8 commits July 21, 2021 16:07

Increased block size up to 4M; txes are allowed to occupy only a subs…

506569a

…et (default half) of the block size while this limit does not apply to certs

Using block priority size for both tx and certs in miner logic

164f3ed

Modifications after code review and some preliminary test with block …

88b796f

…and cert size

Added py test for checking block tx partition handling

f010fe1

Fix to py mc_test for the correct rounding of amounts in proof creati…

dfa69e5

…on; added a py test

Adding an overhead large enough to the max size of compressed bit vec…

4e2f22f

…tor for the case when compressed data are larger than original ones; added UT for bzip2 algo

Fix to mingw configuration to overcome an unresolved symbol in build …

0f442b0

…for windows (issue #152)

Fix to py test after merges

387fc54

alsala force-pushed the as/increase_block_size branch from 1a483cb to 387fc54 Compare July 21, 2021 15:23

cronicc linked an issue Jul 21, 2021 that may be closed by this pull request

Regression in Windows MinGW build #152

Open

alsala added 2 commits July 21, 2021 19:08

Fix path for test data file in py test

d852b84

Fixed sc_block_partitions.py test after merge

7fad5eb

i-Alex approved these changes Jul 29, 2021

View reviewed changes

ptagl suggested changes Jul 30, 2021

View reviewed changes

Modifications to py tests as per code review

06aceff

ptagl approved these changes Aug 4, 2021

View reviewed changes

Import cleanup in a py test

0827e02

ptagl approved these changes Aug 4, 2021

View reviewed changes

albertog78 reviewed Aug 11, 2021

View reviewed changes

alsala commented Aug 27, 2021

View reviewed changes

alsala added 2 commits September 2, 2021 15:22

Merge branch 'sidechains_integration_code_review' into as/increase_bl…

ce8ebc8

…ock_size

Modifications after code review

277eb56

albertog78 approved these changes Sep 2, 2021

View reviewed changes

Cermelli merged commit e5dbbd2 into sidechains_integration_code_review Sep 2, 2021

alsala added a commit that referenced this pull request Sep 8, 2021

Fix for py test after merge with PR #149 which removed certificates m…

2aae35b

…ax priority

alsala mentioned this pull request Sep 8, 2021

As/fix pr183 #185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increased block size up to 4M; txes are allowed to occupy only a subs… #149

Increased block size up to 4M; txes are allowed to occupy only a subs… #149

alsala commented Jul 7, 2021

i-Alex Jul 8, 2021

alsala Jul 8, 2021

alsala Aug 27, 2021

i-Alex Jul 8, 2021

alsala Jul 8, 2021

i-Alex Jul 8, 2021

alsala Jul 8, 2021

cronicc commented Jul 19, 2021 •

edited

Loading

cronicc commented Jul 19, 2021

alsala commented Jul 19, 2021

cronicc commented Jul 19, 2021 •

edited

Loading

i-Alex Jul 29, 2021

albertog78 Aug 11, 2021

alsala Sep 2, 2021

albertog78 Aug 11, 2021

alsala Sep 2, 2021

alsala Aug 27, 2021

alsala Aug 27, 2021

alsala Sep 2, 2021

Increased block size up to 4M; txes are allowed to occupy only a subs… #149

Increased block size up to 4M; txes are allowed to occupy only a subs… #149

Conversation

alsala commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cronicc commented Jul 19, 2021 • edited Loading

cronicc commented Jul 19, 2021

alsala commented Jul 19, 2021

cronicc commented Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cronicc commented Jul 19, 2021 •

edited

Loading

cronicc commented Jul 19, 2021 •

edited

Loading