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 additional partial headers #17788

Conversation

felixbrucker
Copy link
Contributor

Purpose:

Expose farmer peer id, full harvester version, harvester capacity (raw/effective) and plot count when submitting a partial.

Current Behavior:

Currently only the full farmer version and harvester peer id are exposed

New Behavior:

Farmer and harvester peer id and full version, as well as harvester capacity and plot count are exposed.

Testing Notes:

Limitation: capacity and plot count are not scoped by plotnft yet, as those are not tracked separately right now in plot_sync_receivers. The current lookups are fast as its just a direct variable access, scoping per launcher id/contract address would require additional compute resources without a refactor of how plot_sync_receivers works right now.

@felixbrucker felixbrucker requested a review from a team as a code owner March 27, 2024 04:01
@emlowe
Copy link
Contributor

emlowe commented Mar 28, 2024

Interestingly, RFC 6648 (https://www.rfc-editor.org/rfc/rfc6648) deprecates using X- as the beginning nomenclature for new parameters. Perhaps chia should be in the name somewhere - maybe chia-farmer-peer-id

@emlowe emlowe added Added Required label for PR that categorizes merge commit message as "Added" for changelog community-pr labels Mar 28, 2024
@emlowe
Copy link
Contributor

emlowe commented Mar 28, 2024

Some Internal discussion indicates that some of these would be contentious to send by default - I suggest creating a new CHIP to discuss those and they may need to be optional to send

The harvester version itself is likely not controversial - but some of the others eliciting some internal feedback

@felixbrucker
Copy link
Contributor Author

some of these would be contentious to send by default - I suggest creating a new CHIP to discuss those and they may need to be optional to send

which ones exactly?

@danieljperry
Copy link
Contributor

some of these would be contentious to send by default - I suggest creating a new CHIP to discuss those and they may need to be optional to send

which ones exactly?

Farmer peer ID. This might work well as an informational CHIP if there are other clients that could use it. I'm not familiar enough with the pooling protocols to say for sure, though.

@felixbrucker
Copy link
Contributor Author

I don't see how that is contentious in any way, but sure i can create a chip for it

@felixbrucker
Copy link
Contributor Author

The CHIP PR is available here: Chia-Network/chips#114

Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label May 13, 2024
Copy link
Contributor

@almogdepaz almogdepaz left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot removed the stale-pr Flagged as stale and in need of manual review label Jul 11, 2024
@BrandtH22
Copy link
Contributor

Per the conversation in the chip, the inherent security issues with defaulting the additional headers to not be obfuscated, and the reduced version of the pr already adoption; this pr is being closed and not adopted into the chia codebase.

@BrandtH22 BrandtH22 closed this Jul 30, 2024
@felixbrucker felixbrucker deleted the add-additional-partial-headers branch July 30, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog community-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants