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

add Aeon support #67

Merged
merged 9 commits into from
Oct 27, 2017
Merged

add Aeon support #67

merged 9 commits into from
Oct 27, 2017

Conversation

psychocrypt
Copy link
Collaborator

@psychocrypt psychocrypt commented Oct 24, 2017

Add support to mine Aeon and Monero from one miner.

By default the miner is build to support both currencies. It is possible to compile the miner to support Monero or Aeon only.
There is a new command line option --currency and a CMake option XMR-STAK_CURRENCY added.
It was on purpose that the options are named currency and the user can choose between monero and aeon. From the perspective of a non export users there are only different currencies and it could be complicated for to understand that the algorithm behind differ and one is called cryptonight and the other cryptonight-lite.

Review

I have split the changes into logical commits thus each commit can be easily review instead of the hole changes.

Test

  • CPU aeon
  • CPU xmr
  • NVIDIA aeon
  • NVIDIA xmr
  • AMD aeon
  • AMD xmr

Note: I have currently no AMD device available (my test system is dead :-( ) Therefore I tested the OpenCL AMD part only on a NVIDIA card. Could some please test this changes with an AMD device.

};

std::bitset<2> digit;
std::bitset<3> digit;
Copy link
Owner

Choose a reason for hiding this comment

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

You misundestood the code above. Please refer to the comment, we are essentially counting in binary with true / false flags.

So:

false false (00) = 0
false true  (01) = 1
true  false (10) = 2
true  true  (11) = 3

For bitset of 3 you need to have 8 entries in the table, and count up to 8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add some comments to this code part.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I see it now, my bad, this part is ok as-is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are 8 value in the table if XMR-STAK_CURRENCY=all. Else there are 4 values in the table, and bit 3 is always set to 0.

{
std::string tmp;
#if defined(CONF_NO_AEON)
tmp = "xmr";
Copy link
Owner

Choose a reason for hiding this comment

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

Can you change xmr to proper currency name - Monero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I can change it to 'monero'. I used 'xmr' to keep it short but you are right monero could be better.

Repository owner deleted a comment from psychocoderHPC Oct 25, 2017
@fireice-uk
Copy link
Owner

I hate when that happens too ^^

@psychocrypt psychocrypt force-pushed the topic-aeon2 branch 3 times, most recently from f3e577f to 777935c Compare October 26, 2017 19:14
@psychocrypt
Copy link
Collaborator Author

I changed xmr to monero. The writing of monero is case insensitiv to avoid that we get many support tickets because some are writing monero and other Monero

Copy link
Owner

@fireice-uk fireice-uk left a comment

Choose a reason for hiding this comment

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

This is surely a mistake? You seem to have copied the executor singleton into utility.cpp Can you squash the last two commits to make the review easier?

@psychocrypt
Copy link
Collaborator Author

Could you please point to the code- can't find the singleton in utility.hpp or cpp.

@fireice-uk
Copy link
Owner

c791cd9 <- this commit added the executor code (+1000 delta lines)

777935c <- removes the code

Can you squash those two together please?

@psychocrypt
Copy link
Collaborator Author

psychocrypt commented Oct 27, 2017 via email

@fireice-uk
Copy link
Owner

I have an RX480 test rig set up, so will do the AMD testing after the squash

@psychocrypt
Copy link
Collaborator Author

I rebased and squash the PR.

@@ -230,10 +245,30 @@ int main(int argc, char *argv[])
;
configEditor configTpl{};
configTpl.set(std::string(tpl));
std::cout<<"Please enter:"<<std::endl;
auto& currency = params::inst().currency;
if(currency.empty())
Copy link
Owner

Choose a reason for hiding this comment

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

This if is never executed in my testing. After rm *.txt; ./xmr-stak the miner goes on to mine aeon without prompting.

Copy link
Owner

Choose a reason for hiding this comment

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

Just to make sure checked cmake output -> -- Set miner currency to 'monero' and 'aeon'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well spotted: I inserted two logic bugs during the renaming

  • fix with the last commit

tmp = "aeon";
#endif
while(tmp.compare("xmr") != 0 && tmp.compare("aeon") != 0)
while(xmrstak::strcmp_i(tmp, "monero") && xmrstak::strcmp_i(tmp, "aeon"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is wrong must be !condition

#ifndef CONF_NO_XMR
currency.compare("xmr") != 0
#ifndef CONF_NO_MONERO
xmrstak::strcmp_i(currency, "monero")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wrong must be !condition

- add CMake flag description
- announce Monero and Aeon in README.md
i add FAQ section
- cli-miner.hpp:
  - add option `--currency`
  - add guided start section
- config.tpl: add value `currency`
- params.hpp: add value to store the selected currency
- executor.hpp: add dev pool address
... into a single place
- add compile parameter to support aeon and xmr
- update auto suggestion to handle aeon
- add template parameter to kernel to support aeon and xmr
- update auto suggestion
- update auto suggestion default and hwloc
- extent hash function table to support aeon and xmr within one miner
- rename all `xmr` to `monero`
- be insensitive while check for set currency
- add function to compate two strings insensitive
- fix that currency selection is not called (in cli-miner.cpp)
- fix guard to prevent wrong currency selection if compiled for monero or aeon only
@fireice-uk
Copy link
Owner

Ready to merge on my side.

@psychocrypt
Copy link
Collaborator Author

frome my side also

@fireice-uk fireice-uk merged commit c192b97 into fireice-uk:dev Oct 27, 2017
@psychocrypt psychocrypt deleted the topic-aeon2 branch October 27, 2017 19:37
psychocrypt added a commit to psychocrypt/xmr-stak that referenced this pull request Jan 9, 2018
bug was introduced with fireice-uk#67

- increase the L3 sanity check to 2GiB
- fix usage of byte instead of KB
gnagel pushed a commit to gnagel/xmr-stak that referenced this pull request Mar 23, 2019
gnagel pushed a commit to gnagel/xmr-stak that referenced this pull request Mar 23, 2019
bug was introduced with fireice-uk#67

- increase the L3 sanity check to 2GiB
- fix usage of byte instead of KB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants