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

RandomX [DO NOT MERGE] #5549

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@hyc
Copy link
Contributor

commented May 16, 2019

RandomX PoW integration completed. Running successfully on private testnet.
Also requires tevador/RandomX#40 before you can build this. [Already merged, ignore this note.]

You should edit the testnet blockheight and timestamp before running on your own private testnet. Also, I haven't tested yet on Windows, any other Windows devs trying that out would be appreciated.

@hyc hyc force-pushed the LMDB:randomx branch from 9af6db9 to 083b1cb May 16, 2019

@hyc hyc changed the title Randomx [DO NOT MERGE] RandomX [DO NOT MERGE] May 17, 2019

@tevador

This comment has been minimized.

Copy link

commented May 17, 2019

RandomX PR is merged.

@hyc hyc force-pushed the LMDB:randomx branch from 083b1cb to f9a4aeb May 17, 2019

Show resolved Hide resolved src/crypto/slow-hash.c Outdated
Show resolved Hide resolved src/crypto/slow-hash.c Outdated
Show resolved Hide resolved src/rpc/core_rpc_server.cpp Outdated
Show resolved Hide resolved src/cryptonote_core/blockchain.cpp Outdated
Show resolved Hide resolved src/cryptonote_core/blockchain.cpp Outdated
Show resolved Hide resolved src/cryptonote_core/blockchain.cpp Outdated
Show resolved Hide resolved src/crypto/slow-hash.c Outdated
Show resolved Hide resolved src/crypto/slow-hash.c Outdated
Show resolved Hide resolved src/crypto/slow-hash.c Outdated
Show resolved Hide resolved src/cryptonote_core/cryptonote_core.cpp Outdated
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Also needed: a res.pow_algorithm = "RandomX" case in src/rpc/core_rpc_server.cpp, in on_mining_status when major_version >= 12.

@tevador

This comment has been minimized.

Copy link

commented May 17, 2019

BTW we should test that the daemon can correctly recover from a reorg deeper than 64 blocks at the seed height (this can happen in a partition). RandomX cache must be reinitialized with the new block hash or it will cause a chain split.

Show resolved Hide resolved src/crypto/slow-hash.c Outdated
@hyc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

BTW we should test that the daemon can correctly recover from a reorg deeper than 64 blocks at the seed height (this can happen in a partition). RandomX cache must be reinitialized with the new block hash or it will cause a chain split.

Yeah, in the reorg handler will need to invoke a call to invalidate the rs_heights so that the next check will reset the caches. Can try to be clever and only reset if the reorg height spans the seedheight.

Show resolved Hide resolved src/crypto/slow-hash.c Outdated

@hyc hyc force-pushed the LMDB:randomx branch from 93e2e9c to 6f8b43a May 17, 2019

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

When I reach v12 on my private testnet, I start to get the following error

2019-05-17 18:43:53.934 E difficulty overhead.                                             
2019-05-17 18:43:53.935 E Failed to get_block_template(), stopping mining                  
2019-05-17 18:43:53.941 I Found block <70ce2fbfa5a950a1acef4fdbc6a8a26453ba6cceca6f3eb3b5a0b648424b0b71> at height 607 for difficulty: 227393539063790171175524086015817791843
@hyc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@moneroexamples try again now, should be stable.

@hyc

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Hm, forgot to handle the reorg stuff.

Show resolved Hide resolved src/crypto/rx-slow-hash.c Outdated
@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

@hyc

Working now. Noticed significant increase in hr at v12, from 30H to about 300H:

ashrate: 31.7895
hashrate: 29.8947
hashrate: 28.0000
hashrate: 26.1053
hashrate: 24.2105
hashrate: 22.3158
hashrate: 20.4211
hashrate: 18.5263
hashrate: 16.6316
hashrate: 14.8947
hashrate: 13.1579
hashrate: 11.5263
hashrate: 9.7368
hashrate: 18.8421
hashrate: 34.9474
2019-05-17 23:56:04.236 I Found block <6371a13f20f9795882ad2ec2b4ace72e6e83b000ff014eb7f0e13b18fceb774e> at height 24 for difficulty: 12077
hashrate: 49.9474
hashrate: 65.7895
hashrate: 81.5263
hashrate: 98.0526
hashrate: 115.6842
hashrate: 132.8421
status
Height: 25/25 (100.0%) on testnet, mining at 326 H/s, net hash 103 H/s, v12, up to date, 2(out)+1(in) connections, uptime 0d 0h 45m 34s
hashrate: 150.2105
hashrate: 168.0000
hashrate: 185.4737
hashrate: 203.2105
hashrate: 220.8947
hashrate: 238.5263
hashrate: 256.2105
hashrate: 273.9474
hashrate: 291.6842
hashrate: 309.3158
hashrate: 326.9474
hashrate: 333.4737
hashrate: 333.3158
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

Just a note that randomx still is GPL as of that module.

@hyc

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

OK. Moved get_block_longhash out of miner. Couldn't move it to Blockchain due to some circular linking dependencies. Also now has a reorg handler - needs testing.

@hyc hyc force-pushed the LMDB:randomx branch from 2bc927e to b106876 May 18, 2019

@tevador

This comment has been minimized.

Copy link

commented May 18, 2019

Tested on a private testnet. Getting ~4020 H/s in monerod when mining with 8 threads (Ryzen 1700). Looking good.

@hyc hyc force-pushed the LMDB:randomx branch from e3f84ae to f9daab6 May 19, 2019

Show resolved Hide resolved src/crypto/rx-slow-hash.c Outdated
@tevador

This comment has been minimized.

Copy link

commented May 19, 2019

@moneromooo-monero

Just a note that randomx still is GPL as of that module.

It's been updated to the 3-clause BSD license, which should be compatible with Monero.

@hyc hyc force-pushed the LMDB:randomx branch from 3cc216d to 4e22cc7 May 19, 2019

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

rx_vm is a global variable. Is it thread-safe or its users have to take care of synchronizing its access and usage?

@hyc

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

It is thread-local, not global. That's what the THREADV declaration is all about.

@tevador

This comment has been minimized.

Copy link

commented May 27, 2019

rx_vm is a global variable. Is it thread-safe or its users have to take care of synchronizing its access and usage?

randomx_vm is not thread safe. Only one thread may call randomx_calculate_hash at a time using the same randomx_vm instance. It's a good idea to make it thread-local (thread_local keyword in C++11).

The other 2 objects (randomx_cache and randomx_dataset) are thread safe after initialization and may be shared across threads.

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@hyc @tevador

Thanks for info. I will then mutex my calls which use randomx_vm.

@hyc hyc force-pushed the LMDB:randomx branch from 21142b5 to 76debe1 May 28, 2019

@hyc hyc force-pushed the LMDB:randomx branch 6 times, most recently from 05fa571 to 1f1ac37 May 30, 2019

moneromooo-monero and others added some commits May 8, 2019

blockchain: keep alternative blocks in LMDB
Alternative blocks are cleared on startup unless --keep-alt-blocks
is passed on the command line
RandomX integration
Support RandomX PoW algorithm
Proposed tweak for issue with finding seedblock hash
This patch isn't needed if we always restrict block-sync-size to <= SEEDHASH_EPOCH_LAG.
But otherwise, this will allow syncing with larger block-sync-sizes.
Updated to RandomX v1.0.3
This update yields different hashes from previous versions, so any
testnet blockchains using previous code will be invalidated.

@hyc hyc force-pushed the LMDB:randomx branch from f75a96b to 6261ecb Jun 1, 2019

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I notice there are no calls to randomx_release_dataset and randomx_release_cache. Is this intentional?

@hyc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Yes. The cache is needed for as long as monerod is running, so there's no point in ever releasing it. The dataset is needed for as long as mining occurs. While it might make sense to release it when you issue "stop_mining" command, if you then start mining again, it would just impose a needless delay to reallocate it. (And in my own testing, I started and stopped many times, changing the number of threads each time.)

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I don't understand, what is the initial state of R registers for loop execution: https://github.com/tevador/RandomX/blob/master/doc/specs.md#462-loop-execution

In point 1 it says that readReg0 and readReg0 are XORed, and in point 2 it says that further XORing with r0-r7 is performed.

But aren't all those registers 0? At least that what is says in point 4 or initialization:
https://github.com/tevador/RandomX/blob/master/doc/specs.md#461-initialization

Seems to me that R registers maybe are set to something else than 0?

@tevador

This comment has been minimized.

Copy link

commented Jun 4, 2019

@moneroexamples

Maybe you can better understand what is going on by looking at the source code.

https://github.com/tevador/RandomX/blob/bc2aae0f61a5b15472753e0dda399410fd450a4f/src/vm_interpreted.cpp#L215-L238

Line 216 - r0-r7 are set to 0. This is before the loop.
Line 230 - readReg0 and readReg1 are XORed to calculate a random scratchpad address.
Line 237 - r0-r7 are XORed with a random 64-byte scratchpad block.

Lines 230 and 237 are inside the loop. r0-r7 are zero only during the first iteration (i == 0). They will have a non-zero value in other iterations (i == 1..2047). Note that XOR with zero is identity (x ^ 0 == x), so the first iteration actually loads the register values.

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@tevador
Thanks. Regarding step 6 in the loop:

A 64-byte Dataset item at address datasetOffset + mx % RANDOMX_DATASET_BASE_SIZE is prefetched from the Dataset (it will be used during the next iteration).

I can't locate its code. I see steps 5 and 7,8, but can't see the read from datasetOffset + mx:
https://github.com/tevador/RandomX/blob/bc2aae0f61a5b15472753e0dda399410fd450a4f/src/vm_interpreted.cpp#L247-L250

@tevador

This comment has been minimized.

Copy link

commented Jun 5, 2019

@moneroexamples
The interpreted VM skips step 6 since it's too slow to benefit from prefetching anyways. I guess we could add it (or at least a comment) to formally match the specs.

The optimized x86 code performs step 6: https://github.com/tevador/RandomX/blob/bc2aae0f61a5b15472753e0dda399410fd450a4f/src/asm/program_read_dataset.inc#L2-L4

Note that step 6 actually has no effect on the final result. It's present merely to explain the function of the mx register. The prefetched data is not used until the next iteration, where it becomes step 7 (because mx becomes ma due to step 8).

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@tevador

I guess we could add it (or at least a comment) to formally match the specs.

Thanks. I think some comment in step 6 that it is not actually implemented could be useful. The description of step 6, as written now, can lead to some confusion. Or the step could be removed if it does not affect the final result.

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

In table Table 5.2.1 Integer instructions in specs, shouldn't some src be M, for example, IADD_M and ISUB_M?

@tevador

This comment has been minimized.

Copy link

commented Jun 8, 2019

@moneroexamples The "dst" and "src" columns in tables 5.2.1, 5.3.1, 5.4,1 and 5.5.1 specify how the instruction field is decoded according to Table 5.1.2. All memory instructions use the "R" column since the memory address is calculated from an integer register.

The difference between register and memory instructions is that the latter use [mem] operand in the "operation" column.

hyc added some commits Jun 8, 2019

randomx_release_dataset(rx_dataset);
rx_dataset = NULL;
}
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jun 9, 2019

Contributor

Bikeshedding, but "mining" seems out of place in this file.
Also, mutex ?

This comment has been minimized.

Copy link
@hyc

hyc Jun 10, 2019

Author Contributor

It's only called by miner::stop() and only after all mining threads have already been joined, so no mutex is needed.

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Regarding IROL instruction. I don't understand when its called. From what I can tell, since its frequency is 0, macro CASE_REP(IROL_R) expands to nothing, and I can't see in which scenario IROL_R block could be executed.

So is there other scenario under which IROL_R is used?

@tevador

This comment has been minimized.

Copy link

commented Jun 10, 2019

So is there other scenario under which IROL_R is used?

Yes, if RANDOMX_FREQ_IROL_R is set to a non-zero value in configuration.h. For Monero, it's currently set to 0.

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

For Monero, it's currently set to 0

Is there any rational for it to be zero?

@tevador

This comment has been minimized.

Copy link

commented Jun 10, 2019

Is there any rational for it to be zero?

Yes. Rotation left by x is equivalent to rotation right by 64 - x, so it's essentially the same operation. Some architectures (like ARM) don't even have a rotl instruction and it must be emulated.

BTW it's best to open new issues in the RandomX github repository. This PR is related only to RandomX integration into monerod and general questions about RandomX can be confusing for someone attempting a code review.

@moneroexamples

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Thanks for explanation. Will make sure to make issues now, though sometimes not sure where to ask when RandomX was primarly designed for monero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.