[DNM] https://github.com/mimblewimble/grin/issues/3620

[snape479]

This pull request re-enables the VerifierCache and addresses issue: https://github.com/mimblewimble/grin/issues/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.

[trevyn]

Do you know of a way to exercise the full block processing pipeline resulting in an InvalidBlockProof, something along the lines of the test in nrd_validation_rules.rs? :)

[snape479]

Do you know of a way to exercise the full block processing pipeline resulting in an InvalidBlockProof, something along the lines of the test in nrd_validation_rules.rs? :)

Not familiar with that. Wouldn't hurt to add a test at the transaction.rs level though.

[phyro]

@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.

[trevyn]

@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.

[snape479]

Ok, I will take a stab at it.

[antiochp]

This is effectively a duplicate of the changes proposed in https://github.com/mimblewimble/grin/pull/3613 right? I think leveraging DefaultHashable is the most appropriate way of achieving what we want here.

[snape479]

@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.

[trevyn]

@snape479 Nice. Would it be possible to make the grin_secp256k1 modifications locally and produce a short chain that shows the bug, and hardcode that chain data into a test?

[snape479]

That might work. I will take a look at that and see what I can do.

[antiochp]

@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.

One thing we could do here is leverage secp.commit_sum() to build a negative commitment (we can "subtract" a large positive value from a smaller positive value). In fact we can just use this to "invert" a single commitment (by passing it in on the negative side).

    let x = secp.commit_sum(vec![], vec![commit])?;

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.

[snape479]

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.

[trevyn]

@snape479 Awesome! Let me know where to send the bounty award, I’m @trevyn on keybase if you prefer a private channel.

[snape479]

Ok, sent you a message on keybase. There's a proof that the keybase account is connected to this github account.

[trevyn]

Ok, sent you a message on keybase. There's a proof that the keybase account is connected to this github account.

👍🏼. Bounty awarded & payment sent.

@antiochp wants to remove the verifier cache altogether for now in https://github.com/mimblewimble/grin/pull/3628, and we can definitely pull these tests in on top of that.

[antiochp]

ok https://github.com/mimblewimble/grin/pull/3628 is merged. Feel free to either rework this PR to cover just the test or close and open a new PR.

[antiochp]

Closing. Tests included as part of #3630.