Skip to content

Add support for default stratum split#137

Open
rleed wants to merge 5 commits intoOCEAN-xyz:masterfrom
rleed:master
Open

Add support for default stratum split#137
rleed wants to merge 5 commits intoOCEAN-xyz:masterfrom
rleed:master

Conversation

@rleed
Copy link

@rleed rleed commented Jul 17, 2025

The intention of this PR is to add support for a default stratum split. If no tilde is present in the stratum username, one is automatically added at the end of the username it behaves as with a blank modname. If a blank modname exists in the configuration, it will then serve as the default stratum split.

Copy link
Contributor

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

Seems like it would be better to just pass modname_len as 0?

@luke-jr
Copy link
Contributor

luke-jr commented Jul 17, 2025

Also, this means the username mod stuff is always enabled... Probably should have a bool on datum_config that gets set if "" is being used, so we're not wasting CPU on setups that don't use the feature.

@rleed
Copy link
Author

rleed commented Jul 17, 2025

Also, this means the username mod stuff is always enabled...

Doesn't if (datum_config.stratum_username_mod) disable it if it's not configured?

@rleed
Copy link
Author

rleed commented Jul 17, 2025

Seems like it would be better to just pass modname_len as 0?

// CAUTION: modname MUST be part of username_s following a tilde made me wary. The datum_stratum_mod_username function indexes the tilde character without checking that it exists first, which I think can't be assumed when just passing modname_len as 0.

@rleed
Copy link
Author

rleed commented Jul 18, 2025

Also, this means the username mod stuff is always enabled...

Doesn't if (datum_config.stratum_username_mod) disable it if it's not configured?

Ok, now I got it. I made the changes to skip the new logic if there is no default mod in the configuration.

@rleed rleed requested a review from luke-jr July 18, 2025 11:37
@rleed
Copy link
Author

rleed commented Jul 19, 2025

Seems like it would be better to just pass modname_len as 0?

// CAUTION: modname MUST be part of username_s following a tilde made me wary. The datum_stratum_mod_username function indexes the tilde character without checking that it exists first, which I think can't be assumed when just passing modname_len as 0.

I've now modified the existing code to remove the caution above and pass a len of 0.

@luke-jr luke-jr added the enhancement New feature or request label Jul 22, 2025
@rleed
Copy link
Author

rleed commented Sep 18, 2025

Can somebody move this forward?

const char *modname, *addr;
json_t *moddefn, *proportion_j;
bool at_least_one_mod = false;
datum_config.stratum_default_mod_present = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This function shouldn't have side effects / be accessing globals.

Comment on lines -896 to +898
const char * const tilde = &modname[-1];
const char * const tilde = strchr(username_s, '~');
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather not do this strchr twice... Kinda ugly, but maybe just pass modname=&username_s[json_string_length(username) + 1] in client_mining_submit? (Since length is zero, it should be safe?)

const size_t modname_len = json_string_length(username) - (modname - username_s);
if (tilde || datum_config.stratum_default_mod_present) {
const char * const modname = tilde? &tilde[1]: "";
const size_t modname_len = strlen(modname);
Copy link
Contributor

Choose a reason for hiding this comment

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

strlen should be unnecessary

@luke-jr luke-jr added this to the v0.5.0 milestone Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants