Skip to content
This repository has been archived by the owner on May 27, 2020. It is now read-only.

Initial version of coinbase check for cgminer #638

Closed
wants to merge 8 commits into from

Conversation

4tar
Copy link

@4tar 4tar commented Aug 18, 2014

When do pool mining, users get job from the pool by being pushed stratum mining_notify message or firing GBT message, however if the pool was compromised by some malicious hackers then the job might generate no benefit for users. By introducing the coinbase checking function in the client, user can ensure that (how much of) the benefits, if any, is going to the expected address, so that greatly reduces the risk of wasting their hashing power. From another side, a pool that always generates coinbases passing users' check can give its client enough confidence to trust it.

The usage is pretty straightforward, three arguments are added, say --cbaddr, --cbtotal, and -cbperc, all are per-pool settings. --cbaddr specifies the address user want to check against in the coinbase txn, --cbtotal indicates the total amount of benefit the coinbase should include, while --cbperc tells the percentage in all benefit the --cbaddr specified address should obtain.

If any problem found in the coinbase data, this patch would suspend the pool for 10 minutes from generating workload. Note if none of these arguments was input by user, this patch would still work but just check the coinbase is integral regarding the standard format.

Here is the typical usage: cgminer -u XX -p YY -o stratum+tcp://thepool.com --cbaddr POOL_ADDR --cbtotal '>2499999999' --cbperc '>0.99'; the command would check whether the total amount output coins in the coinbase txn is more than 2499999999, and the POOL_ADDR would receive more that 99% of that.

Both "--cbtotal" and "--cbperc" arguments need a '>'/'+', '<'/'-', or '=' prefix to tell what kind of comparing the user want to check against the coinbase. Note when using '>' or '<', quotation marks ’ or " would be needed to avoid confusing the shell with I/O redirection, for convenience '+' or '-' can be used to replace '>' or '-', e.g. the above sample can be rewrite like: cgminer -u XX -p YY -o stratum+tcp://thepool.com --cbaddr POOL_ADDR --cbtotal +2499999999 --cbperc +0.99.

Besides that "--cbperc" obviously depends on "--cbaddr", the three new args can combines as users' will without any other restriction.

Patches including similar functions have also be submitted to other mining client.

Signed-off-by: Huang Le 4tarhl@gmail.com

Signed-off-by: Huang Le <4tarhl@gmail.com>
@@ -2998,7 +3002,6 @@ static void removepool(struct io_data *io_data, __maybe_unused SOCKETTYPE c, cha
return;
}

pool->enabled = POOL_DISABLED;
Copy link

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

The following remove_pool() will set the status.

@kanoi
Copy link

kanoi commented Aug 19, 2014

Personally - I see no point to this change.
It will also require some sizeable testing since it is quite invasive in the stratum code.

My idea on this a while ago was to simply have the option of reporting the intial and change details in the coinbase (or report if no transactions are in the coinbase) however to actually act on that information seems kneejerk.
Most large pools payout based on finding blocks, so if you mine at a pool that hides the details of the block finds and payouts, then you are mining at the wrong pool.
The pool's wallet is often unknown, and is not what one would check to determine one's own expected payout, simply look at the blocks that have been found themselves.

Removing that unknown by optionally reporting information about the coinbase would have pluses, however I'm unsure why the change would be more than some extra code to examine the coinbase transaction rather than how it seems to be rather invasive code changes inside stratum itself.

Bitcoin devs were all gungho about trying to stop address reuse not all that long ago, and I'd not be surprised if many pools do change addresses regularly.
The 'error' condition is actually in common use and would occur way more often on a valid pool than the rarity of a pool being hacked and passing out bad work, so it seems a pointless change.
It seems more like an attempt (that would most likely produce many false positives) to do the security work of the pool that the pool itself should be handling.

@4tar
Copy link
Author

4tar commented Aug 19, 2014

@kanoi Thanks for the open discussion. We do have a case requiring such a feature: a large mining farm owner now wanna detach from those huge pool to do "solo" mining at the farm scale, and he manages to sign a contract with a pool operator to setup an independent pool only for his farmer, and according to the contract all rewards should go directly to his address. Now he need to ensure from his side that everything works fine.

The case is not imaginary, we have seen this requirement happening around us, more and more farm owner ask for higher mining transparency, for lower risk when pool is being hacked, and also many of them like to contribute their farms to increase the meaningful nodes but decrease, which in turn bring good to their mining future.

Even without those cases, having a coinbase check is not a bad idea. When the user input none of these three arguments, the patch would just check to ensure the coinbase format integrity, so that eliminate possible error caused by network issue or simply pool server coding error (yes, we also saw this happened).

0. Support the pay-to-pubkey script
1. Support address list instead of a single address for parameter --cbaddr, multiple addresses are separated by ',' or '+'
2. Remove the '<' and '=" comparison, since what we really want is just '>=', so that we can simply the code a little bit
3. Convert the caculated percentage to float to avoid annoying precision problem during comparing;
4. Others...

Signed-off-by: Huang Le <4tarhl@gmail.com>
… script

Signed-off-by: Huang Le <4tarhl@gmail.com>
… argumment description

Signed-off-by: Huang Le <4tarhl@gmail.com>
Signed-off-by: Huang Le <4tarhl@gmail.com>
…to the previous-defined pool

Signed-off-by: Huang Le <4tarhl@gmail.com>
…implify the code

Signed-off-by: Huang Le <4tarhl@gmail.com>
Signed-off-by: Huang Le <4tarhl@gmail.com>
@ckolivas
Copy link
Owner

Is this feature still desired? If so, are there any updates to this patch? I'd like to see no testing done by default to not confuse users unless the user explicitly enables it on the command line.

@ckolivas ckolivas closed this Feb 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants