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

Support for third-party, farmer-rewarded, Harvesters #16717

Merged
merged 47 commits into from Jan 26, 2024

Conversation

harold-b
Copy link
Contributor

@harold-b harold-b commented Oct 27, 2023

This adds support for third-party Harvesters to be able to overwrite the farmer reward puzzle hash when it is time for them to take their reward, given a winning block.

  • Harvester overwrite address
    • Test overwrite address is respected and the UnifinishedBlock includes it
    • Test correct fields for data source are included when the Harvester requests them.
  • introduce convention for agreement between Harvester & Farmer as to when the Harvester is allowed to take the farmer reward by overwriting the reward address.
    • Add test for Harvester-Farmer reward convention

- [x] install_timelord = True must be enabled for new tests Timelord test requirement removed.

@madMAx43v3r
Copy link

Great stuff, but there's one issue, the harvester cannot blindly sign any message hash, it needs the original data for all messages that need to be signed:

  • FoliageBlockData for the reward block, and the RewardChainBlockUnfinished structure (corresponding to the unfinished_reward_block_hash)
  • optional FoliageTransactionBlock in case of transaction blocks
  • VDFInfo for NewSignagePoint.challenge_chain_sp (from full_node_protocol.RespondSignagePoint.challenge_chain_vdf.output)
  • VDFInfo for NewSignagePoint.reward_chain_sp (from full_node_protocol.RespondSignagePoint.reward_chain_vdf.output)
  • PostPartialPayload for partials

If the harvester has an API to blindly sign message hashes, that API can be used to get a signature for a block header that does not give the dev fee.

@harold-b
Copy link
Contributor Author

Good to see you, here @madMAx43v3r. I was going to reach out to you this week, so I'm happy to see that you're already involved here.

I was assuming the validation of what was being signed would be determined by the harvester given the plot_identifier and challenge_hash used by the winning proof of space, but we can definitely send back the original payload for each hash being signed. I think that makes it simpler.

Please let me know if you have any more feedback.

@madMAx43v3r
Copy link

Ok so let me simplify the problem. Right now we essentially have an API for signing a message hash (array index 0) and an API for signing a data structure that can be validated (array index 1).

What stops someone from using the API that doesnt validate anything and just signs the message hash for the message that is supposed be validated? You can simply bypass the validation.

So yeah, unfortunately the harvester will need all the data structures to be signed in original form. Maybe they can be passed in serialized form, ie. byte array, to simplify the protocol.

Since the message type information is not hashed, we have to rely on the serialized byte count to determine what kind of message it is. Not prepending a type string for the message hashes is really dangerous, but there's nothing we can do about it now, it's part of the consensus already.

Even if you don't serialize the messages, you can still spoof the API into not validating the message that should be validated by using another message type and making it serialize to the same bytes, so the hash is what you need to be signed. This only works if another message has the same serialized size of course. Just saying, if the type information was included in the hash, this attack would not work. So now we have to be careful about message size collisions. If the message to be validated has a dynamic field that can take any size, like a string or byte array, then this whole idea will not work... Remember the CAT1 debacle was an attack of the same nature.

For the random dev fee lottery I would just hash the ProofOfSpace.challenge and ProofOfSpace.proof together and use the first or last byte as a random number. The harvester then also needs to send a 8-bit number to the farmer / node to specify the fee threshold. When the random 8-bit is <= the fee threshold given by the harvester the fee should be paid. Also need a boolean to disable dev fee of course.

@madMAx43v3r
Copy link

madMAx43v3r commented Oct 31, 2023

Looking at the code again now, the message to be validated is FoliageBlockData, which has one optional field, so it can have two different sizes. Not ideal but we might get lucky that no other message possibly can have one of those sizes:

FoliageBlockData can be 133 or 229 bytes (with pool signature)
FoliageTransactionBlock is 168 bytes
ClassgroupElement is 100 bytes
PostPartialPayload is over 300 bytes, so not an issue
ChallengeChainSubSlot can be between 144, and 220 bytes (~20 possible combinations of optional fields)
RewardChainSubSlot can be 174 or 206 bytes

RewardChainBlockUnfinished is not relevant because it's not being signed, but is still needed to validate FoliageBlockData.

@harold-b
Copy link
Contributor Author

Yes FoliageBlockData is the only one I included initially as to ensure that the overwritten reward address was respected in the hash value that is to be signed. The idea was for the Harvester to maintain a state whenever it found a proof and wanted to take the reward. Then rigidly reject signing anything that did not follow the expected signing flow for that proof that it found.

But i's not important now since it's certainly reasonable to simplify it given your recommendation to include all the data, so I already made those changes yesterday. Anything being sent to the Harvester to sign will also contain the original data used to generate the hash that will be signed. The Harvester just need to request the original data be included when it sends its proof of space to the Farmer.

I still have to update the tests to reflect the changes, though

@madMAx43v3r
Copy link

Then rigidly reject signing anything that did not follow the expected signing flow for that proof that it found.

I don't think that would work. For example I could replace the signature request for the partial with the FoliageBlockData for a block that does not pay the fee, and present the expected FoliageBlockData to the harvester. So the harvester will see what it expects for the FoliageBlockData, but I will use the other signature I got via the fake partial to publish a block that doesn't pay the fee.

As long as there exists any path where you can get a signature for any hash, it allows to bypass the fee.

The Harvester just need to request the original data be included when it sends its proof of space to the Farmer.

So a harvester with dev fee would then always set that bool to true right?

@madMAx43v3r
Copy link

madMAx43v3r commented Oct 31, 2023

Just saw your other commit. I missed ChallengeChainSubSlot and RewardChainSubSlot before, let's check their serialized message sizes.

ChallengeChainSubSlot can be between 144, and 220 bytes (~20 possible combinations of optional fields)
RewardChainSubSlot can be 174 or 206 bytes

So got lucky again, no collision with FoliageBlockData.

In my implementation I will not sign any message that is 133 or 229 bytes and is not a validated FoliageBlockData, for security reasons.

@harold-b
Copy link
Contributor Author

I don't think that would work.

I think you're right. The partial payload would make it troublesome.

For the random dev fee lottery I would just hash the ProofOfSpace.challenge and ProofOfSpace.proof together and use the first or last byte as a random number...

That sounds good, is that what you currently do? We decided to let the Harvester itself take care of that without any convention agreement. We would simply display the stats to the user. The Harvester provider would not get locked-in into any convention this way and simplifies things from the Farmer side.

So a harvester with dev fee would then always set that bool to true right?

Yes, I also changed it so that a bool field can be specified here so that the farmer would include signing data even if the reward address will not be overwritten. Though overwriting the reward address also implies sending back source data for signing.

Is there anything else that sticks out to you as troublesome from the current changes?

@madMAx43v3r
Copy link

That sounds good, is that what you currently do?

No

We decided to let the Harvester itself take care of that without any convention agreement

That means an optional address field for the fee address in the proof response? I'm fine with that.

Yes, I also changed it so that a bool field can be specified here so that the farmer would include signing data even if the reward address will not be overwritten.

That's needed for sure, I wont sign anything without hashing all the data myself. Doing some if-else magic there is just asking for a loophole.

@harold-b
Copy link
Contributor Author

harold-b commented Nov 1, 2023

That means an optional address field for the fee address in the proof response? I'm fine with that.

Yes. Probably also specified whenever RespondSignatures is sent back to continue the chain. This avoid having to add stateful data to the Farmer

@harold-b
Copy link
Contributor Author

harold-b commented Nov 1, 2023

FYI @madMAx43v3r, it's supposed to be ClassgroupElement for the signing data source instead of VDFInfo here. Updated in last commit

@madMAx43v3r
Copy link

Yeah you right, those are 100 bytes, all good.

@madMAx43v3r
Copy link

madMAx43v3r commented Nov 2, 2023

was assuming the validation of what was being signed would be determined by the harvester given the plot_identifier and challenge_hash used by the winning proof of space

There's a lot of challenge hashes and it's easy to get confused:

  • the static challenge for 64 SPs in a row
  • the challenge chain VDF output hash for each SP
  • the individual challenge for each plot (hashed with plot id)

I'm not sure if you can simply change the challenge_hash or plot_identifier in RequestSignatures and still get the needed signature while bypassing the fee. After all those fields are not being hashed / signed directly.

The proper way would be to supply the RewardChainBlockUnfinished data structure together with FoliageBlockData so it can be directly verified what proof and pos_ss_cc_challenge_hash is being signed, without any state magic.

Looking at the code now, plot_identifier is needed to fetch the private key, but challenge_hash is simply returned back to the caller, so it can be set to any value (same for sp_hash).

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Nov 7, 2023
Copy link
Contributor

github-actions bot commented Nov 7, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

.gitignore Outdated Show resolved Hide resolved
chia/harvester/harvester_api.py Show resolved Hide resolved
chia/full_node/full_node_api.py Show resolved Hide resolved
chia/full_node/full_node.py Outdated Show resolved Hide resolved
chia/protocols/farmer_protocol.py Outdated Show resolved Hide resolved
@nossdpool
Copy link

Update on the "convention" mentioned above:
No convention will be used. Instead, the Harvester would decide itself when to take its fee. The farmer will respect its decision without doing any checks on its side. Instead only stats would be presented to the user.

Why have you decided to simplify it this way? Instead of just gathering statistics there can be a defined algorithm how to determine which block reward is going to be claimed by a harvester. FoliageBlockData refer to RewardChainBlockUnfinished which in turn contains ProofOfSpace and challenge_chain_sp which hash can be used as a deterministic source of randomness for example. This way farmer and a harvester can use the same decision algorithm and fee percentage will be real and not just "statistics". Most users wouldn't even able to mine enough blocks to gather statistics and a harvester can cheat in many ways, on some machines it will take small percent on another machine it will take bigger percent and attribute it all to bad luck. Lets just use pseudo random hashes and make reward block claims deterministic.

@harold-b
Copy link
Contributor Author

harold-b commented Nov 7, 2023

Why have you decided to simplify it this way?

I'll tag-in @wjblanke and @arvidn for further discussion on that

@madMAx43v3r
Copy link

I can confirm some people get paranoid when they are unlucky with the fee lottery.

@arvidn
Copy link
Contributor

arvidn commented Nov 8, 2023

Why have you decided to simplify it this way?

Requiring a specific mechanism to be used to determine which blocks have their reward diverted adds complexity and limits the freedom of the harvester. If one of you guys come up with some new clever scheme of picking blocks to divert, you would need an update to the protocol.

The point of doing this, as you point out, would be that both the farmer and the harvester could check each other. However, it's not obvious what the best action would be, if a discrepancy is discovered.

Refusing to sign, and squander the block-win doesn't help anyone. The farmer loses the fees and pool reward, the harvester loses its reward and the network loses netspace. It's kind of the "doomsday machine"-option; incentivize conformance through mutual destruction. I also think it has questionable value.

  1. The farmer running a 3rd party harvester already implicitly trusts it.
  2. I would think, by far, the most likely cause of a discrepancy would be a bug

@nossdpool
Copy link

come up with some new clever scheme of picking blocks

In current design the most obvious clever scheme is to cheat on user. For example a difference between 3% fee and a 5% fee is so tiny that only a really big miner can trace with a high confidence but for a harvester author it is a huge difference.

The algorithm can be very simple and straightforward. Get the lower 32 bit of an sp_hash and compare it with a value fee * UINT_MAX.

The farmer running a 3rd party harvester already implicitly trusts it

Not having a strict and deterministic reward distribution algorithm opens a room for social engineering attacks on competitors. Fanboys of a harvester author A will spam all over the internet that a harvester B is cheating and claiming bigger fee than it declares. We want to focus on programming and not spend resources on SMM.

haorldbchi and others added 18 commits January 26, 2024 16:44
harvester and farmer for test_missing_messages`
Inject pre-generated signage points and blocks instead.
- Update valid fee quality test to be unit, instead of pseudo-integration.
- New test for invalid fee quality.
- More explicit messaging for invalid fee qualities.
- Explicitly comment out uncovered case in test.
- Update types for mypy.
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jan 26, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Starttoaster Starttoaster merged commit 0ae3b28 into main Jan 26, 2024
265 checks passed
@Starttoaster Starttoaster deleted the hb.harvester-update branch January 26, 2024 23:07
@madMAx43v3r
Copy link

madMAx43v3r commented Jan 27, 2024

Are you sure this is what Max needs for GH? Would be good to be certain so that we know we're implementing what's necessary.

Anyone who wants to implement this CHIP will need it, since it's needed to verify what reward address has been set in the foliage block. It needs to be passed together with FoliageBlockData which contains the unfinished_reward_block_hash to the 3rd party harvester.

EDIT: Small correction, RewardChainBlockUnfinished is needed to verify the fee logic, it contains the plot challenge as well as the proof.

Just for reference, I've said this multiple times before (3 months ago):

RewardChainBlockUnfinished is not relevant because it's not being signed, but is still needed to validate FoliageBlockData.

The proper way would be to supply the RewardChainBlockUnfinished data structure together with FoliageBlockData so it can be directly verified what proof and pos_ss_cc_challenge_hash is being signed, without any state magic.

@harold-b
Copy link
Contributor Author

OK sorry I missed it, I'll implement it

@danieljperry
Copy link
Contributor

@madMAx43v3r @nossdpool
This PR is included in our latest Release Candidate build:
https://github.com/Chia-Network/chia-blockchain/releases/tag/2.2.0-rc1
Feel free to test its functionality in your pools.

Additional work is being completed in PR #17435, which we intend to merge soon.

@danieljperry
Copy link
Contributor

@madMAx43v3r @nossdpool The code for CHIP-22 has all been merged. 2.2.0-rc3 is the first build to contain the complete CHIP. Please test it with your pools.
https://github.com/Chia-Network/chia-blockchain/releases/tag/2.2.0-rc3

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 Farming full_node harvester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants