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.

Sign up for GitHub

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

Jump to bottom

Fix payment processor #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix payment processor #310

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion libs/paymentProcessor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,8 @@ function SetupForPool(logger, poolOptions, setupFinished){
var test = Object.keys(workers);
var addressAmounts = {};
var totalSent = 0;
var splitted_address = [w].toString();
splitted_address = splitted_address.split('.',1);
test.forEach(function(w) {
daemon.cmd('validateaddress', [w], function (results) {
var validWorkerAddress = results[0].response.isvalid;
Expand All @@ -406,6 +408,7 @@ function SetupForPool(logger, poolOptions, setupFinished){
var toSend = (worker.balance + worker.reward) * (1 - withholdPercent);
if (toSend >= minPaymentSatoshis) {
var address = worker.address = (worker.address || getProperAddress(w));
var address = address.split('.',1);
worker.sent = addressAmounts[address] = satoshisToCoins(toSend);
worker.balanceChange = Math.min(worker.balance, toSend) * -1;
totalSent += toSend;
Expand Down Expand Up @@ -577,7 +580,7 @@ logger.info(logSystem, logComponent, addressAmounts);


var getProperAddress = function(address){
if (address.length === 40){
if (address.length === 60){
Copy link
Member

Choose a reason for hiding this comment

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

Whys this changed?

Copy link
Author

Choose a reason for hiding this comment

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

That portion of code doesn't even make sense to exist anymore, since miners should be allowed to setup a longer .workername.

It's also broken, and doesn't even change the address to poolconfig pooladdrress.

Cheers

Copy link

@mooleshacat mooleshacat Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I agree partially with @LEAD-Anoy74 on the issue of the length should not even be checked.

The function is not meant to change the address at all... It is meant to check the address if it is valid and matches pool configuration.

Most pools accept WALLET.WORKER format and the workername can be any length (unless pool limits it)

What I would do is split the address / worker name and then do the length check after, or just rip it out alltogether.

Either use:

var getProperAddress = function(address){

    var splitted_address = [address].toString();
    splitted_address = splitted_address.split('.',1);

    if (splitted_address.length === 60){
            return util.addressFromEx(poolOptions.address, splitted_address);
    fi
    else return splitted_address;

}

or use this:

var getProperAddress = function(address){

    return util.addressFromEx(poolOptions.address, address);

}

I would personally modify the function and add more sanity / validation checks.

For example:

  • Add in a check to make sure there are no symbols inside the string containing the address/worker name (except for the '.' separating the two)
  • Add a validateaddress RPC call to check the address is valid
  • Use the same validateaddress RPC call results to check the address is owned by the pool daemon

Copy link
Author

Choose a reason for hiding this comment

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

Correct

Copy link
Member

Choose a reason for hiding this comment

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

The spacing of this is all over the place, but from a quick skim, the function can be kept the same by splitting the address before its passed into getProperAddress. The purpose of this function is as a sanity check to ensure that only a valid base58 bitcoin-type address is passed in and used from that point forward. This change would be breaking that check.

return util.addressFromEx(poolOptions.address, address);
}
else return address;
Expand Down