[DNM] https://github.com/mimblewimble/grin/issues/3620 #3621
Conversation
|
Do you know of a way to exercise the full block processing pipeline resulting in an |
Not familiar with that. Wouldn't hurt to add a test at the transaction.rs level though. |
|
@antiochp I remember you were discussing an option where a block validation would not use any cache, did you guys agree on what the next steps would be? I think having block validation that ignores cache might be a good idea. We can optimize this later when blocks are actually heavy and we have a lot more eyes on the code. |
|
@snape479 If you're interested in trying, I can (personally) offer a bounty of US$1,000 paid in BTC for a test at the block/chain level mentioned above that exercises the inflation bug without reaching into the verifier cache directly. |
|
Ok, I will take a stab at it. |
antiochp
changed the title
|
This is effectively a duplicate of the changes proposed in #3613 right? |
|
@trevyn I checked in a test that demonstrates an invalid range proof at the block level. What it doesn't do is show the inflation bug directly. To do that you need to create a negative output to offset a positive output. That would require a change to the grin_secp256k1 library c code, as the function currently accepts a uint64_t type. I don't suggest doing that just for this test. If you want the test as it stands, I can do a separate pull request for that since it sounds like there's another solution for the verifier_cache itself. |
|
@snape479 Nice. Would it be possible to make the |
|
That might work. I will take a look at that and see what I can do. |
chain/tests/mine_simple_chain.rs
Outdated
| .unwrap(); | ||
|
|
||
| // overwrite our rangeproof with a rangeproof from last block | ||
| tx2.body.outputs[1].proof = tx1.body.outputs[0].proof; |
There was a problem hiding this comment.
We probably want to ensure we are explicit in which rangeproof is being replaced and which one we are replacing it with. The order (outputs[1] etc.) will not necessarily be consistent and we may accidentally pick one that would cause the validation to fail for unrelated reasons (swapping out a coinbase one for example).
chain/tests/mine_simple_chain.rs
Outdated
| @@ -817,6 +817,89 @@ fn output_header_mappings() { | |||
| clean_output_dir(".grin_header_for_output"); | |||
| } | |||
|
|
|||
| /// Test the duplicate rangeproof bug | |||
| #[test] | |||
| fn test_duplicate_rangeproof() { | |||
chain/tests/mine_simple_chain.rs
Outdated
| // process_block fails with verifier_cache disabled or with correct verifier_cache | ||
| // implementations | ||
| assert_eq!( | ||
| chain.process_block(next, chain::Options::SKIP_POW).is_ok(), |
There was a problem hiding this comment.
We just merged #3616 to master and we now propagate up a more useful error on failure here.
We should test for the error explicitly as part of this test - it should be an InvalidBlockProof.
We don't want to accidentally have this test passing because of some unrelated error occurring during block processing.
One thing we could do here is leverage Might be a lot of hoops to jump through to set the offending block up but this would be the perfect opportunity to put this in place. |
|
Yes, commit_sum worked. I was able to invert the output and generate 1 million grin without modifying the secp library. The test is working for me. In checkins before disabling the verifier_cache, it fails and since that checkin it passes. As I mention in the TODO there's probably a better way to check error, but I notice that errors changed in master, so when I merge it over to the other PR, I'll see if I can improve that somehow. |
|
Ok, sent you a message on keybase. There's a proof that the keybase account is connected to this github account. |
|
ok #3628 is merged. |
|
Closing. Tests included as part of #3630. |
This pull request re-enables the VerifierCache and addresses issue: #3620
The added test can be used against the previous version of the code to verify that it fails without the associated fix in this PR.