Fix: correct usage of BitMask in multicore sdr

[qy3u]

This PR contains two commits:

1. Use the correct node index to set bpm. The prev node is the first node of parents not the sixth:

   rust-fil-proofs/storage-proofs-core/src/drgraph.rs

   Line 168 in 00cb804

   ApiVersion::V1_1_0 => (0, &mut parents[1..]),

2. Unset the bpm after using it. The current implemention has never unsetted it so it will always read all base parents in the main hash loop.

[porcuquine]

Good catches. I think this is good, but should probably go a little further (as noted in comments).

@cryptonemo We should make sure to run some full lifecycle tests on this (when done) to ensure there are no surprises.

[qy3u]

Hi @porcuquine. This PR has updated.Now,it stores the api_version in ParentCache and CacheReader, then it matchs the version stored to get corresponding indexs.

[cryptonemo]

Thanks @qy3u -- I'll look closer and get some testing going on this early next week.

[dignifiedquire]

@cryptonemo can you run this for some performance checks, making sure we don't have any accidental regressions

[cryptonemo]

@cryptonemo can you run this for some performance checks, making sure we don't have any accidental regressions

Yes, this is on the list!

[cryptonemo]

Still have wider testing to do, but the first 32GiB seal lifecycle test completed in 320m. Compared to master taking 347m on the same hardware. Testing again to see if this is consistent or something else.

[porcuquine]

The update appears to accomplish what I requested, and as far as I can see this all seems good.

This should not affect verification of historical proofs, even though the changes do relate to the previous upgrade. This PR is just propagating some changes from that upgrade which were not reflected in sealing.

The large performance impact is likely due to resetting bpm between node iterations.

In addition to the tests for performance regression, we should make sure to repeatedly observe that sealing completes consistently and correctly (i.e. valid PoRep, and subsequently PoSt — though I am less concerned about PoSt) are produced. I don't have specific reason to think something will have gone wrong, but there is an operational change to how the data is being processed, and if for whatever reason that is introducing an error, this is where it would show up. It could be the case that such an error be non-deterministic. Given that the performance change results in much faster sealing, we should be skeptical in principle — even though, as noted, I believe this is probably a good and safe change.

Therefore, I approve, but contingent on a clean history of multiple runs with no failures. If we were to see any failures in testing, we should not just disregard and re-run the tests. We are not looking for presence of success but rather absence of failure.

Good work.

[cryptonemo]

@qy3u Any chance you can back out the merge I did to bring this up to latest master? I'd prefer to keep your original 3 commits and have those rebased against the latest master branch and pushed back here. If you're able to do that today, I'd appreciate it -- otherwise, I'll keep what we have after that merge and finish testing on it.

[cryptonemo]

This appears to test well, as-is.

[cryptonemo]

After merging this, CI tested well. However, after cutting a new release with it, a CI test failed that exercised this code path (seal_lifecycle 2k). Based on that alone, it's possible this will be reverted in our next shipped version.