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

pool: Use regular expressions to identify clients. #406

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Oct 11, 2023

This modifies that logic that identifies mining clients by their user agent to use regular expressions instead of requiring hard coded version numbers.

It also adds tests to ensure proper matching functionality.

The primary motivation is to allow the pool to continue to work with new patch versions of mining clients without requiring updates the pool software.

However, it also provides the flexibility to adapt to specific client software that might not conform to semver semantics in the future if is desired.

@davecgh davecgh force-pushed the identify_miner_by_regex branch from f2e4ecd to 4b3e62e Compare October 11, 2023 05:14
pool/miner_id.go Outdated Show resolved Hide resolved
@davecgh davecgh force-pushed the identify_miner_by_regex branch 2 times, most recently from 61ef5ad to 065813f Compare October 11, 2023 20:34

// miningClients maps regular expressions to the supported mining client IDs
// for all user agents that match the regular expression.
miningClients = map[*regexp.Regexp][]string{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
miningClients = map[*regexp.Regexp][]string{
miningClients = map[*regexp.Regexp]string{

I think this change would serve to simplify things quite a bit. Maybe there is a reason for it, but I don't see why a single ID would need to be mapping to multiple miner types. We can add support for that later if it ever becomes necessary.

Copy link
Member Author

@davecgh davecgh Oct 12, 2023

Choose a reason for hiding this comment

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

I left it that way because there is infrastructure for upgrading based on the hash rate. There is nothing forcing two different devices to use a different user agent, and as a result, you can have multiple devices with wildly different hashrates identifying themselves the same.

There are tests which cover the upgrading as well (see testClientUpgrades).

Copy link
Member Author

@davecgh davecgh Oct 12, 2023

Choose a reason for hiding this comment

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

As an aside, I think that ideally the pool really generally shouldn't even be assigning hash rates and instead should figure it out dynamically. It would be similar to how the client upgrade code is doing for devices with the same UA, but instead it would allow it to become more or less difficult depending on share submission rate.

Then, it could assign some initial hash rates per UA and even better would be for the protocol to allow clients to specify it themselves (which is unfortunately not part of stratum). Naturally, it would need to be sanity checked and only serve as an initial starting point for the aforementioned dynamic behavior.

However, that is a fair amount of work that doesn't seem worth it given the goal at the moment is primarily getting into shape for BLAKE3 and gominer as opposed to adding new features.

This modifies that logic that identifies mining clients by their user
agent to use regular expressions instead of requiring hard coded version
numbers.

It also adds tests to ensure proper matching functionality.

The primary motivation is to allow the pool to continue to work with new
patch versions of mining clients without requiring updates the pool
software.

However, it also provides the flexibility to adapt to specific client
software that might not conform to semver semantics in the future if is
desired.
@jholdstock jholdstock merged commit 0bb2494 into decred:master Oct 13, 2023
2 checks passed
@davecgh davecgh deleted the identify_miner_by_regex branch October 13, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants