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

ArticMine's proposal for block size constraints #5124

Merged
merged 1 commit into from Mar 4, 2019

Conversation

@moneromooo-monero
Copy link
Contributor

commented Feb 3, 2019

No description provided.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch from 485a2f0 to 9c3d9f8 Feb 6, 2019

@iamsmooth

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Can we get a comment describing the intended algorithm or a link to such a document.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch from 9c3d9f8 to 27e5623 Feb 6, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

ArticMine's original spec is now included in the commit message.

@iamsmooth

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

I assume that 2) "store the LongTermBlockWeight of a block unencrypted in the block itself" from the commit message spec is not implemented since that is not needed (did not see any code doing this either). Is that correct?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Indeed, it is not stored in the block itself (it would require verification anyway, so would not save CPU), but it is stored in the block info in the db (hence why there is another migration).

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch from 27e5623 to edd2345 Feb 9, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

More tests, and fixes.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch 3 times, most recently from 5b404f1 to 524f276 Feb 14, 2019

if (m_long_term_block_weights_height + 1 > m_long_term_block_weights.capacity())
{
uint64_t block_height = m_long_term_block_weights_height - m_long_term_block_weights.capacity() + 1;
m_long_term_block_weights.push_front(block_height);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 18, 2019

Contributor

Aren't we supposed to add the long term block weight corresponding to block_height here? Seems strange to add height to m_long_term_block_weights.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 18, 2019

Author Contributor

Wow, it's amazing this still passes the tests :S

fprintf(stderr, "Failed to update cumulative weight limit 2\n");
exit(1);
}
if(0)if (!bc->update_next_cumulative_weight_limit())

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 18, 2019

Contributor

Forgotten to be dropped?

exit(1);
}
uint64_t last = bc->get_db().get_block_long_term_weight(bc->get_db().height() - 1);
if (last != ltw) std::cout << "Inconsistency at " << h << std::endl;

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 18, 2019

Contributor

I don't see why the indentation should be irregular for these lines.

short_term_constraint = *long_term_effective_median_block_weight + *long_term_effective_median_block_weight * 2 / 5;

weights.clear();
get_last_n_blocks_weights(weights, CRYPTONOTE_REWARD_BLOCKS_WINDOW);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 18, 2019

Contributor

This causes the following error when size_t can't be reduced to uint64_t:

/Users/buildbot/slave/monero-static-osx-10_12/build/src/cryptonote_core/blockchain.cpp:3856:31: error: non-const lvalue reference to type 'vector<size_t>' cannot bind to a value of unrelated type 'vector<uint64_t>'
    get_last_n_blocks_weights(weights, CRYPTONOTE_REWARD_BLOCKS_WINDOW);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 18, 2019

Author Contributor

That got supposedly fixed a couple days ago, though I did it blind...

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 18, 2019

Author Contributor

Nevermind, looks like this fix did not make it to this branch. Fixing...

@stoffu

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

@moneromooo-monero:

Indeed, it is not stored in the block itself (it would require verification anyway, so would not save CPU), but it is stored in the block info in the db (hence why there is another migration).

But @ArticMine's original description

Note: To avoid possible consensus issues over rounding the LongTermBlockWeight for a given block should be calculated to the nearest byte, and stored as a integer in the block itself. [...]
2) There is the requirement to store the LongTermBlockWeight of a block unencrypted in the block itself. [...]

seems to imply that there's a modification to the block structure. I still think the commit log should be modified in some manner. (I'd simply drop them with @ArticMine's consent.)

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

It's equivalently stored in the database block structure. There was not much point in adding to the block itself given it would have had to be recalculated anyway for verification.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch 3 times, most recently from cba9851 to 81a600c Feb 18, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

I changed the commit message to say that the long term weight is only stored in the db, not the actual block.

@@ -3098,7 +3119,8 @@ bool Blockchain::check_fee(size_t tx_weight, uint64_t fee) const
uint64_t needed_fee;
if (version >= HF_VERSION_PER_BYTE_FEE)
{
uint64_t fee_per_byte = get_dynamic_base_fee(base_reward, median, version);
const bool use_long_term_median_in_fee = version >= HF_VERSION_LONG_TERM_BLOCK_WEIGHT;
uint64_t fee_per_byte = get_dynamic_base_fee(base_reward, use_long_term_median_in_fee ? m_long_term_block_weights.back() : median, version);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 19, 2019

Contributor

m_long_term_block_weights contains LongTermBlockWeight defined as min(BlockWeight,1.4*LongTermEffectiveMedianBlockWeight) which doesn't represent any sort of median; it's just the actual block weight being capped by some slowly moving number, so it can change quite abruptly over consecutive heights. Is this really intended?

if (weights.empty())
weights.resize(1);
weights[0] = long_term_block_weight;
long_term_median = epee::misc_utils::median(weights);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 19, 2019

Contributor

Why is the computation of long term median performed twice? What does weights[0] = long_term_block_weight; mean?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 19, 2019

Author Contributor

It's done once over the last N, then once over the last N-1 plus the current one. weights[0] = long_term_block_weight just removes the oldest element, and adds the newest one.

get_last_n_blocks_weights(weights, CRYPTONOTE_REWARD_BLOCKS_WINDOW);

uint64_t short_term_median = epee::misc_utils::median(weights);
uint64_t effective_median_block_weight = std::min<uint64_t>(std::max<uint64_t>(CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V5, short_term_median), CRYPTONOTE_SHORT_TERM_BLOCK_WEIGHT_SURGE_FACTOR * *long_term_effective_median_block_weight);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 19, 2019

Contributor

std::max<uint64_t>(CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V5, short_term_median) seems redundant given the subsequent line m_current_block_cumul_weight_median = full_reward_zone;.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 19, 2019

Author Contributor

True, I'll still leave it that way because it's self contained, and does not break if that other code gets changed.

// check the new values are the same as the old ones
for (int i = -2; i < std::min(add, remove); ++i)
{
ASSERT_EQ(bc->get_db().get_block_long_term_weight(h0 + i), old_ltbw[i + 2]);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 19, 2019

Contributor

I don't see this holding necessarily. I've confirmed the issue by making the following change

-    int add = (n * 23) % 12;
+    int add = (n * 23) % 123;

which results in the following error:

[ RUN      ] long_term_block_weight.pop_invariant_random
/monero/tests/unit_tests/long_term_block_weight.cpp:329: Failure
Value of: old_ltbw[i + 2]
  Actual: 440230
Expected: bc->get_db().get_block_long_term_weight(h0 + i)
Which is: 440020
[  FAILED  ] long_term_block_weight.pop_invariant_random (4232 ms)

Or is the intent to assert that the values remain unchanged only when add tends to be not significantly larger than remove?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 19, 2019

Author Contributor

No, it should always work :/

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 19, 2019

Contributor

Why do this test? The blocks added will have different (randomized) weights than the removed blocks, AIUI.

Edit: it was clarified in a private chat.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch from 81a600c to 11b309c Feb 19, 2019

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch 2 times, most recently from 0e629f2 to 574bd62 Feb 27, 2019

ArticMine's new block weight algorithm
This curbs runaway growth while still allowing substantial
spikes in block weight

Original specification from ArticMine:

here is the scaling proposal
Define: LongTermBlockWeight
Before fork:
LongTermBlockWeight = BlockWeight
At or after fork:
LongTermBlockWeight = min(BlockWeight, 1.4*LongTermEffectiveMedianBlockWeight)
Note: To avoid possible consensus issues over rounding the LongTermBlockWeight for a given block should be calculated to the nearest byte, and stored as a integer in the block itself. The stored LongTermBlockWeight is then used for future calculations of the LongTermEffectiveMedianBlockWeight and not recalculated each time.
Define:   LongTermEffectiveMedianBlockWeight
LongTermEffectiveMedianBlockWeight = max(300000, MedianOverPrevious100000Blocks(LongTermBlockWeight))
Change Definition of EffectiveMedianBlockWeight
From (current definition)
EffectiveMedianBlockWeight  = max(300000, MedianOverPrevious100Blocks(BlockWeight))
To (proposed definition)
EffectiveMedianBlockWeight  = min(max(300000, MedianOverPrevious100Blocks(BlockWeight)), 50*LongTermEffectiveMedianBlockWeight)
Notes:
1) There are no other changes to the existing penalty formula, median calculation, fees etc.
2) There is the requirement to store the LongTermBlockWeight of a block unencrypted in the block itself. This  is to avoid possible consensus issues over rounding and also to prevent the calculations from becoming unwieldy as we move away from the fork.
3) When the  EffectiveMedianBlockWeight cap is reached it is still possible to mine blocks up to 2x the EffectiveMedianBlockWeight by paying the corresponding penalty.

Note: the long term block weight is stored in the database, but not in the actual block itself,
since it requires recalculating anyway for verification.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:amblock branch from 574bd62 to b8787f4 Mar 4, 2019

@fluffypony
Copy link
Collaborator

left a comment

Reviewed

@fluffypony fluffypony merged commit b8787f4 into monero-project:master Mar 4, 2019

0 of 2 checks passed

buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fluffypony added a commit that referenced this pull request Mar 4, 2019
Merge pull request #5124
b8787f4 ArticMine's new block weight algorithm (moneromooo-monero)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.