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

Even more minimal version #4

Closed
wants to merge 47 commits into from
Closed

Conversation

stoffu
Copy link

@stoffu stoffu commented Feb 11, 2018

This is a variation of my earlier PR #2 with the main difference being that I didn't bother changing most of the constants in cryptonote_config.h such as CURRENT_TRANSACTION_VERSION and CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V2 in favor of even smaller changes. The main benefit of this is that the existing tests pass without much changes. Except that I observe a strange RingCT verification failure on core tests, which I'm investigating now. EDIT: Fixed now. All the tests pass.

The main changes regarding the hardfork is condensed into a single commit 21c2492 while the rest are more compact and self-contained.

Also, testnet is supported with two seed nodes I've set up (162.210.173.150:21180 & 162.210.173.151:21180).

Because the way Aeon did the 592000 hardfork was quite different from how Monero handles hardforks, I made the following compromise: the 592000 hardfork is still seen as running on the same network version 1, but the difficulty target changes after the height 592000. This means that the testnet chain also needs to follow the same path as the main chain with the same hardfork at the height 592000. EDIT: This isn't necessarily the case; the switch to the 240s diff target gets triggered either when the protocol version is incremented to v2 or the block height reaches 592000.

Going forward, we should adopt the more flexible hardfork mechanism of Monero.

@ghost
Copy link

ghost commented Feb 13, 2018

I think you're approach is best. Especially things like not renaming every constant from MONERO to AEON. if we don't take a minimal approach, then rebasing again is going to be another big project because of the merge conflicts that will be present everywhere.

@wyattp11
Copy link

Build success on El Capitan first try, thank you.

@stoffu stoffu force-pushed the aeon-rebase-min branch 2 times, most recently from 9e8dcd1 to 112b3a9 Compare February 14, 2018 09:56
@@ -86,11 +86,12 @@ namespace cryptonote {
return CRYPTONOTE_MAX_TX_SIZE;
}
//-----------------------------------------------------------------------------------------------
bool get_block_reward(size_t median_size, size_t current_block_size, uint64_t already_generated_coins, uint64_t &reward, uint8_t version) {
bool get_block_reward(size_t median_size, size_t current_block_size, uint64_t already_generated_coins, uint64_t &reward, uint8_t version, size_t height) {

Choose a reason for hiding this comment

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

uint64_t height

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -167,38 +167,17 @@ namespace cryptonote
ADD_CHECKPOINT(1000000, "46b690b710a07ea051bc4a6b6842ac37be691089c0f7758cfeec4d5fc0b4a258");

Choose a reason for hiding this comment

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

Testnet checjpoint ought to be removed too, no ?

Copy link
Author

Choose a reason for hiding this comment

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

ok

const int target_minutes = target / 60;
const int emission_speed_factor = EMISSION_SPEED_FACTOR_PER_MINUTE - (target_minutes-1);
assert(target_minutes & (target_minutes - 1) == 0); // assume target_minutes to be power of 2
const int emission_speed_factor = EMISSION_SPEED_FACTOR_PER_MINUTE - (int)std::log2(target_minutes);

Choose a reason for hiding this comment

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

This might be OK in practice, but I'd make my own that works on powers of 2 instead of using a floating point lib function.

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -1045,15 +1038,15 @@ namespace cryptonote
crypto::secret_key encrypt_key(crypto::secret_key key, const epee::wipeable_string &passphrase)
{
crypto::hash hash;
crypto::cn_slow_hash(passphrase.data(), passphrase.size(), hash);
crypto::cn_slow_hash_2m(passphrase.data(), passphrase.size(), hash);

Choose a reason for hiding this comment

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

These aren't used for anything in aeon yet. Maybe using the light variant is prefered in these cases ?

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -119,7 +119,7 @@ namespace cryptonote {
return !carry;
}

difficulty_type next_difficulty(std::vector<std::uint64_t> timestamps, std::vector<difficulty_type> cumulative_difficulties, size_t target_seconds) {
difficulty_type next_difficulty(std::vector<std::uint64_t> timestamps, std::vector<difficulty_type> cumulative_difficulties, size_t target_seconds, size_t height) {

Choose a reason for hiding this comment

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

uint64_t height

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -969,7 +969,7 @@ namespace cryptonote
}
//---------------------------------------------------------------------------------
//TODO: investigate whether boolean return is appropriate
bool tx_memory_pool::fill_block_template(block &bl, size_t median_size, uint64_t already_generated_coins, size_t &total_size, uint64_t &fee, uint64_t &expected_reward, uint8_t version)
bool tx_memory_pool::fill_block_template(block &bl, size_t median_size, uint64_t already_generated_coins, size_t &total_size, uint64_t &fee, uint64_t &expected_reward, uint8_t version, size_t height)

Choose a reason for hiding this comment

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

uint64_t height

Copy link
Author

Choose a reason for hiding this comment

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

ok

if (m_core.get_testnet())
{
// TODO: relace (uint64_t)-1 with (H-1) where H is the v2 fork height on testnet. Or remove this line altogether if H >= HARDFORK_1_HEIGHT
last_block_v1 = std::min<uint64_t>(last_block_v1, (uint64_t)-1);

Choose a reason for hiding this comment

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

That's a noop isn't it ?

Copy link
Author

@stoffu stoffu Feb 14, 2018

Choose a reason for hiding this comment

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

As in the comment, my intention is to replace (uint64-t)-1 later if we decide to introduce a v2 testnet fork at a height lower than HARDFORK_1_HEIGHT . So it is a no-op currently, but it might change in the future.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated this part slightly so that it's a bit simpler.

@@ -57,7 +57,7 @@ bool gen_rct_tx_validation_base::generate_with(std::vector<test_event_entry>& ev
miner_accounts[n].generate();
CHECK_AND_ASSERT_MES(generator.construct_block_manually(blocks[n], *prev_block, miner_accounts[n],
test_generator::bf_major_ver | test_generator::bf_minor_ver | test_generator::bf_timestamp | test_generator::bf_hf_version,
2, 2, prev_block->timestamp + DIFFICULTY_BLOCKS_ESTIMATE_TIMESPAN * 2, // v2 has blocks twice as long
2, 2, prev_block->timestamp + DIFFICULTY_BLOCKS_ESTIMATE_TIMESPAN * 4, // v2 has blocks four times as long

Choose a reason for hiding this comment

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

That seems predicated on height though, not block version AFAICT, from your code above

Copy link
Author

@stoffu stoffu Feb 14, 2018

Choose a reason for hiding this comment

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

As in get_block_reward bf320b8#diff-1a57d4e6013984c420da98d1adde0eafR91 , I made a little trick in the logic such that the block time becomes V2 (i.e. 240s) either when:

  1. the network version is 1 2, or
  2. the block height reaches HARDFORK_1_HEIGHT .

Because we explicitly set the hf version to 2, the block time should be 240s. Do I make sense?

Choose a reason for hiding this comment

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

Yes

std::vector<tx_destination_entry> destinations;
destinations.push_back(td);
destinations.push_back(td);
destinations.push_back(td);
destinations.push_back(td); // 30 -> 7.39 * 4
destinations.push_back(td); // 70 -> 17.39 * 4

Choose a reason for hiding this comment

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

Out of curiosity, I'm guessing something broke with those initial values, what was it ?

Copy link
Author

@stoffu stoffu Feb 14, 2018

Choose a reason for hiding this comment

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

The block reward on V2 fork doubled due to the block time changing from 120s to 240s, so the numbers needed to be adjusted.

@@ -64,7 +64,7 @@ int main(int argc, char *argv[]) {
}
uint64_t res = cryptonote::next_difficulty(
vector<uint64_t>(timestamps.begin() + begin, timestamps.begin() + end),
vector<uint64_t>(cumulative_difficulties.begin() + begin, cumulative_difficulties.begin() + end), DEFAULT_TEST_DIFFICULTY_TARGET);
vector<uint64_t>(cumulative_difficulties.begin() + begin, cumulative_difficulties.begin() + end), DEFAULT_TEST_DIFFICULTY_TARGET, 1);

Choose a reason for hiding this comment

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

n + 1 ? Guess it doesn't matter as long as < threshold though

Copy link
Author

Choose a reason for hiding this comment

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

I'd rather have it this way, since the height is used to conditionally make an adjustment to the output diff value; see the end of next_difficulty():

    difficulty_type new_diff = (low + time_span - 1) / time_span;
    if (height >= HARDFORK_1_HEIGHT && height < HARDFORK_1_HEIGHT+HARDFORK_1_DIFFADJ_WINDOW) {
      new_diff += new_diff*(HARDFORK_1_HEIGHT+HARDFORK_1_DIFFADJ_WINDOW-height)*(HARDFORK_1_DIFFADJ-1)/HARDFORK_1_DIFFADJ_WINDOW;
    }
    return new_diff;

It doesn't matter because the maximum height stored in tests/difficulty/data.txt is only 29000, but the test may fail if someone added some bad numbers to data.txt. I'd rather have the test completely ignore this adjustment by always setting the height to 1.

Copy link
Author

Choose a reason for hiding this comment

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

because the maximum height stored in tests/difficulty/data.txt is only 29000

I was wrong here, 29000 represents a timestamp. The height corresponds to the number of lines in data.txt, which is 1000. Anyway, the reason of not using n+1 as the height parameter to next_difficulty remains the same.

@stoffu stoffu force-pushed the aeon-rebase-min branch 4 times, most recently from 00557ba to 4033e3c Compare February 14, 2018 23:40
 * src/cryptnote_config.h: The constant `config::testnet::GENESIS_TX` was
changed to be the same as `config::GENESIS_TX` (the mainnet's transaction)
because the mainnet's transaction was being used for both networks.

* src/cryptonote_core/cryptonote_tx_utils.cpp: The `generate_genesis_block` function
was ignoring the  `genesis_tx` parameter, and instead it was using the `config::GENESIS_TX`
constant. That's why the testnet genesis transaction was changed. Also five lines of unused
code were removed.

Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>
@stoffu stoffu force-pushed the aeon-rebase-min branch 4 times, most recently from d5906a8 to b41251e Compare February 15, 2018 01:58
@iamsmooth
Copy link

iamsmooth commented Feb 15, 2018

Noting for the benefit of people contributing to this PR and the previous one that the current plan is to declare this method the winner both for bounty purposes (as decided by bounty sponsors) and also accepted for eventual adoption as the main AEON codebase.

Other submissions will be declared runners up, will also receive a portion of the bounty, and are encouraged to submit their value-add, if any, as individual PRs following the conclusion of the rebase task.

@wyattp11
Copy link

Will we be voting on a name for the rebase? Since previous was Phoenix... first mention of Phoenix was by Hesiod? Or move on from/to another mythical creature?

@ghost
Copy link

ghost commented Feb 17, 2018

What can we do to finalize the rebase? Do we need to update readme files, do more testing, etc? Not rushing, just making sure there's nothing else we can do.

@stoffu stoffu force-pushed the aeon-rebase-min branch 4 times, most recently from 2cae7f8 to 5bb306b Compare March 21, 2018 05:31
stoffu and others added 20 commits March 21, 2018 14:48
…ine below-std fee transactions only half the time (1e743dc)
This is the first variant of many, with the intent to improve
Monero's resistance to ASICs and encourage mining decentralization.
Remove CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V5
Remove DYNAMIC_FEE_PER_KB_BASE_FEE_V5
Set BLOCKS_SYNCHRONIZING_DEFAULT_COUNT to 200
Giving ring size 0 (whether explicitly or implicitly) means using wallet's
saved default ring size. If the saved default ring size is also 0, the
predefined DEFAULT_RING_SIZE (=3) is used.
@stoffu
Copy link
Author

stoffu commented Mar 21, 2018

@iamsmooth
Agreed about keeping the per-kb fee. The relevant commits have been dropped.

@stoffu
Copy link
Author

stoffu commented Apr 1, 2018

Closing and moving to https://github.com/stoffu/monero/tree/aeon-rebase-new so that the rebase point is at the latest master of Monero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants