- 
                Notifications
    
You must be signed in to change notification settings  - Fork 36
 
fork!: Change PoW algorithm to make proposal-switching free #727
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
Conversation
| 
           Missing/unfinished logic: 
 Tests missing: 
  | 
    
224617d    to
    854d98a      
    Compare
  
    aecdff5    to
    391971c      
    Compare
  
    e494276    to
    b043835      
    Compare
  
    | 
           Problem with current approach @aszepieniec. I think this line allows guessers to guess on multiple proposals at once. Which we probably don't want: let buffer_hash = Tip5::hash_pair(self.root, leaf_prefix);Whole validation algorithm:     pub(super) fn validate(
        self,
        auth_paths: PowMastPaths,
        target: Digest,
        consensus_rule_set: ConsensusRuleSet,
        parent_digest: Digest,
    ) -> Result<(), PowValidationError> {
        let leaf_prefix = match consensus_rule_set {
            ConsensusRuleSet::Reboot => auth_paths.commit(),
            ConsensusRuleSet::HardforkAlpha => parent_digest,
        };
        let buffer_hash = Tip5::hash_pair(self.root, leaf_prefix);
        let (index_a, index_b) = Self::indices(buffer_hash, self.nonce);
        let (leaf_a, leaf_b) = if consensus_rule_set == ConsensusRuleSet::Reboot {
            (
                Self::leaf(leaf_prefix, index_a),
                Self::leaf(leaf_prefix, index_b),
            )
        } else {
            let index_a = u64::from(Self::bitreverse(
                index_a.try_into().unwrap(),
                Self::MERKLE_TREE_HEIGHT as u32,
            ));
            let index_b = u64::from(Self::bitreverse(
                index_b.try_into().unwrap(),
                Self::MERKLE_TREE_HEIGHT as u32,
            ));
            (
                Self::leaf(leaf_prefix, index_a),
                Self::leaf(leaf_prefix, index_b),
            )
        };
        if !MTree::verify(self.root, index_a as usize, &self.path_a, leaf_a) {
            return Err(PowValidationError::PathAInvalid);
        }
        if !MTree::verify(self.root, index_b as usize, &self.path_b, leaf_b) {
            return Err(PowValidationError::PathBInvalid);
        }
        let pow_digest = auth_paths.fast_mast_hash(self);
        let meets_threshold = pow_digest <= target;
        if !meets_threshold {
            return Err(PowValidationError::ThresholdNotMet);
        }
        Ok(())
    }I suggest we change the problematic line to: let buffer_hash = Tip5::hash_pair(self.root, auth_paths.commit());
 For transaction throughput it's important that guessers are incentivized to guess on the most valuable proposals (and not just any) as that's the best way to incentivize the mining of transactions.  | 
    
          
 Motivated by this I added a new required test above: Ensure that opened indices into the Merkle tree change when the proposal changes, not just when the parent block changes.  | 
    
…lpha In ConsensusRuleSet::Reboot the opened indices depends on the concrete proposal being guessed on. But because of a bug in 89fac40 and in 4554764, this was not the case on HardFork-alpha. Concretely an opened Merkle tree authentication path could be used for as many block proposals as was known. This would have been very bad for transaction throughput since the solution would have been to an arbitrary proposals with the right parent. With this fix, the indices of the opened leafs depend on the concrete proposal. But the Merkle tree stays unchanged, it still only depends on the parent block's hash. See #727: #727 (comment) Add test: Ensure that indices cannot be reused over multiple proposals.
…lpha In ConsensusRuleSet::Reboot the opened indices depends on the concrete proposal being guessed on. But because of a bug in 89fac40 and in 4554764, this was not the case on HardFork-alpha. Concretely an opened Merkle tree authentication path could be used for as many block proposals as was known. This would have been very bad for transaction throughput since the solution would have been to an arbitrary proposals with the right parent. With this fix, the indices of the opened leafs depend on the concrete proposal. But the Merkle tree stays unchanged, it still only depends on the parent block's hash. See #727: #727 (comment) Add test: Ensure that indices cannot be reused over multiple proposals.
d221457    to
    fdbf05f      
    Compare
  
    …lpha In ConsensusRuleSet::Reboot the opened indices depends on the concrete proposal being guessed on. But because of a bug in 89fac40 and in 4554764, this was not the case on HardFork-alpha. Concretely an opened Merkle tree authentication path could be used for as many block proposals as was known. This would have been very bad for transaction throughput since the solution would have been to an arbitrary proposals with the right parent. With this fix, the indices of the opened leafs depend on the concrete proposal. But the Merkle tree stays unchanged, it still only depends on the parent block's hash. See #727: #727 (comment) Add test: Ensure that indices cannot be reused over multiple proposals.
d1da7b2    to
    66af7df      
    Compare
  
    | 
           Ready for review. I suggest we activate the block height at height 15.000.  | 
    
…lpha In ConsensusRuleSet::Reboot the opened indices depends on the concrete proposal being guessed on. But because of a bug in 89fac40 and in 4554764, this was not the case on HardFork-alpha. Concretely an opened Merkle tree authentication path could be used for as many block proposals as was known. This would have been very bad for transaction throughput since the solution would have been to an arbitrary proposals with the right parent. With this fix, the indices of the opened leafs depend on the concrete proposal. But the Merkle tree stays unchanged, it still only depends on the parent block's hash. See #727: #727 (comment) Add test: Ensure that indices cannot be reused over multiple proposals.
66af7df    to
    307f8a2      
    Compare
  
    Mining without peers is disabled in this application since we don't want users to waste energy unless they're reasonably sure that they are on the tip. Checking if they have any peers is a very good heuristic for catching the negative case. However, to make some integrations/end-to-end tests easier to perform, without needing connected peers, this commit limits this rule to only apply on main net. In other words: On any other network than main, you can now mine without having any peers. This rule is specifically intended to facilitate an end-to-end test on the testnet network that the node's software handles both mining and verification correctly across the upcoming alpha hardfork. The correct transition across a hardfork can then be tested by resetting the block to hard fork height minus two (118 on testnet, as hardfork occurs at 120 on testnet), and then starting the client with both guessing and composition active.
The helper function that returns the child of current tip, which is both passing the validity check and has a valid PoW is pulled out to the tests/shared module such that it can be used elsewhere.
Add a test that verifies that the peer loop handles the transition from `ConsensusRuleSet::Reboot` to `ConsensusRuleSet::HardforkAlpha` correctly.
Only peer loop may do this.
…lpha In ConsensusRuleSet::Reboot the opened indices depends on the concrete proposal being guessed on. But because of a bug in 89fac40 and in 4554764, this was not the case on HardFork-alpha. Concretely an opened Merkle tree authentication path could be used for as many block proposals as was known. This would have been very bad for transaction throughput since the solution would have been to an arbitrary proposals with the right parent. With this fix, the indices of the opened leafs depend on the concrete proposal. But the Merkle tree stays unchanged, it still only depends on the parent block's hash. See #727: #727 (comment) Add test: Ensure that indices cannot be reused over multiple proposals.
307f8a2    to
    644c1e6      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. I want to note that I'm not as much in-the-know as others.
…lpha In ConsensusRuleSet::Reboot the opened indices depends on the concrete proposal being guessed on. But because of a bug in 85e6799 and in 8410301, this was not the case on HardFork-alpha. Concretely an opened Merkle tree authentication path could be used for as many block proposals as was known. This would have been very bad for transaction throughput since the solution would have been to an arbitrary proposals with the right parent. With this fix, the indices of the opened leafs depend on the concrete proposal. But the Merkle tree stays unchanged, it still only depends on the parent block's hash. See #727: #727 (comment) Add test: Ensure that indices cannot be reused over multiple proposals.
Verify that the hardfork actually happens when this test expects it to.
Verify that blocks following hardfork-alpha rule set is rejected when rule set is reboot. Co-authored-by: Ferdinand Sauer <[email protected]>
for some reasone this was commented out in 8410301. Add again.
295803f    to
    8c1fedf      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to file preprocess_task.rs? It contained some useful interfacing between async and non-async code that will be difficult to reproduce. Unfortunately, it seems to have been rebased away.
lib.rs:8: #![cfg_attr(feature = "track-lock-location", feature(async_fn_track_caller))] Does not compile on stable rust.
Under HFa rules, preprocessing is independent of the block proposal. The only requirement is the prev_block_digest. As a result:
- preprocessing can start before the block proposal is finished (not necessarily good);
 - you can switch to another block proposal for free (necessarily good: high tx confirmation rate).
 
The indices depend on a) the PoW MAST paths, and on b) the nonce.
We want to ensure that every hash invocation occurring inside the tight loop fixes the nonce, the proposal, and the guesser buffer. If we ensure these qualities then there is no way for the clever optimizer to reuse (or save) hashes across different iterations.
The index picker preimage depends on the guesser buffer Merkle root and the proposal's MAST auth paths. The indices depend on that and the nonce.
Therefore:
- If the predecessor block is different, then the guesser buffer is different, and consequently the root is different. To obtain paths valid relative to this root, the miner must store two buffers or recompute on the fly the missing parts of the paths.
 - If the predecessor block is the same but the proposal is different (up to PoW data), then the index picker preimage is different, which results in different path indices being sampled with reasonably high probability (more on this later).
 
If the miner uses the same nonce to guess on two different proposals which have the same predecessor, and there happens to be a collision in the path indices, then the bulk of the hashes in the tight loop can be reused. Specifically, the encoding of Pow is identical and so is its hash. I count 7 permutations instead of 39. In the case of two proposals, the probability of this event is small enough to not care: 2^-58. However, a malicious strategy involves sitting on a large number k of proposals and computing the path indices for all k proposals takes 8 hashes (?) per index pair. Bear with me for some probability calculations.
Of the 2^58 possible index pairs, each one has probability (1-2^-58)^k of not being sampled at all, and one minus that gives the probability of any given index pair being sampled once or more. Multiply that with the total number of index pairs to get the expected number of unique pairs after sampling k times with repetition: 2^58 * (1 - (1 - 2^-58)^k). So the expected number of collisions is k - 2^58 * (1 - (1 - 2^-58)^k).
k = 2^ 1  -> E[#coll] =  3.46944695195361418882384896278381347656250000000000000000000000000000000000000000000000000e-18
k = 2^ 10  -> E[#coll] =  1.81721304670645407755600038204345509918940712495986238547234553052233997780675469727477754e-12
k = 2^ 20  -> E[#coll] =  1.90734681382078349612633122391242953432722558829097633772290943850314942173699061499155706e-6
k = 2^ 30  -> E[#coll] =  1.99999999565382799437962544667008217171070416879076367096722052605292644172618887807248906
k = 2^ 40  -> E[#coll] =  2.09714933333396912154662710620707230176903836932520861762517803150974286467752614993385633e6
So after collecting k = 2^30 proposals we may reasonably expect one colliding pair of indices. If we enter into the tight guessing loop with these two proposals then we can spend 39 hashes to trial one (proposal, nonce) pair, and 7 more hashes to trial the second.
| guess_nonce_iteration(&guesser_buffer, threshold, rng, &sender) | ||
| }) | ||
| .map_init( | ||
| || rng.clone().unwrap_or(std_rng_from_thread_rng()), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you convince me that this generates a different RNG per rayon thread?
| 
           Focusing on the algorithmic part of your feedback, @aszepieniec: The picked indices are derived from a commitment to everything in the proposal apart from the two authentication paths in the  The precalculation required to set up such an attack seems very expensive. Each index pair costs 64 hash invocations to calculate. And I don't think you'll find bigger collisions than two nonce/proposal pairs. I'm not sure it can ever be worth it. And what is the benefit, that you get the 2nd guess cheaper than the 1st one? However, if we want to mitigate this attack, we could use the   | 
    
| 
           Feedback received on another channel: 
 Not sure we are going to accommodate the last wish of theirs. At least not through this PR. But it's definitely an optimization that should be done in the future, that the composer automatically creates the most valuable block proposal (is already happening), without doing redundant work (is not happening).  | 
    
| 
           Discussing PR with @aszepieniec, regarding his attack outlined above: 
  | 
    
Co-authored-by: Ferdinand Sauer <[email protected]> Co-authored-by: Alan Szepieniec <[email protected]>
c752c04    to
    d1e6a37      
    Compare
  
    | 
           Re. @aszepieniec 
 You are right. But isn't that OK? If you want to use this debugging feature, use nightly. What's the problem with that? Also: This PR isn't about this feature, so that line should not be changed here. Mind you: neptune-core can still be built on stable, just not with that feature enabled. 
 We might be able to dig it out from   | 
    
177dcc1    to
    d1e6a37      
    Compare
  
    Ensure same pow-solution is always found. Otherwise some tests cannot reuse transaction/block proofs in which case they either take hours to run, or they fail on CI.
| 
           Tested on two machines connected to each other on testnet-0. Before upgrade to this branch: After upgrade:  | 
    
b694167    to
    407ce3c      
    Compare
  
    Add one block with a transaction from the mempool to the canonical test verifying that the things work as expected on hardfork-alpha.
407ce3c    to
    02c6a6d      
    Compare
  
    
Change the PoW algorithm in order to increase transaction throughput.
A small algorithmical change to the PoW algorithm which constitutes a hard-fork since old versions of the software will consider new blocks invalid.
The current PoW algorithm has a preprocessing step that takes ~12 seconds on a very fast GPU and ~40 seconds on a fast CPU. This preprocessing step is invalidated by a new block proposal, meaning that the guessers are heavily disincentivized from switching block proposals while mining. The preprocessing step involves building a large Merkle tree, and it is the leafs of this Merkle tree that depends on the concrete block proposal. With this commit, the leafs instead depend only on the previous' block's hash which means that switching between block proposals is free as long as the parent of the two proposals is the same.
The reason that this change is good for transaction throughput is that it makes it much more likely that the competitive miner will switch to whatever proposal that pays them the highest fee. And including merging another fee-paying transaction into the block proposal increases the reward for the guesser. In the scenario where a mining pool decides both the block proof and the guessser reward (and pays out users for their near-misses) this change also benefits transaction throughput since the preprocessing step (being up to 42GB) will be done locally at each user, and if they can reuse the preprocessed data for a new block proposal/task sent by the mining pool, the mining pool is more likely to update the task regularly as new transactions are merged into the coinbase.
How to verify that this works:
hard_fork_alphain theneptune-core/src/protocol/consensus/consensus_rule_set.rsfile.hardfork_alpha_happy_casein theneptune-core/src/application/loops/peer_loop.rsfile.neptune-core/src/protocol/consensus/block/pow.rsto verify actual bitreversing of the indices under the new rule set.neptune-coreontestnet-0where the harfork transition happens at block height 120 (and not 15,000 as it does on main net). This must be done on a single-proof capable machine. A machine with at least 64GB RAM.73f7da9a0dbc6fbadd3f64ea4f65f16f4fe223a66a04e7da03011e7ca88f1b6be703442c4e270000). This can be achieved by connecting to peer51.15.139.238:19798:neptune-core --network testnet --peer 51.15.139.238:19798