Allow sealing without t_aux files #1717

vmx · 2023-09-14T16:17:48Z

This PR introduces a new feature called fixed-rows-to-discard, when enabled, no t_aux files are written or read. It contains quite a lot of changes as some refactorings lead to less StoreConfigs and hence make it easier to reason about the whole change that it still does the correct thing.

This PR supersedes #1715.

filecoin-proofs/src/api/mod.rs

vmx · 2023-09-19T13:03:42Z

As requested on a side-channel, let's start with a PR with the refactorings first. That can be found at #1719.

vmx · 2023-09-20T08:00:05Z

I've put this into draft state, as it'll be split into several PRs. Once those a merged, I'll rebase that one and mark it then again ready for review.

vmx · 2023-10-24T10:41:10Z

Ready for review. It's different from the precious version as all the refactors leading to this PR were already merged. From the commit message:

If the fixed-rows-to-discard feature is enabled, then TreeR has a fixed
number of rows discarded (2). There is not option to change that value.
This also means that we no longer need to persist the TemporaryAux file
t_aux. This code path will neither read nor write such a file.

DrPeterVanNostrand · 2023-10-24T19:32:41Z

filecoin-proofs/src/api/util.rs

+    let t_aux_path =
+        merkletree::store::StoreConfig::data_path(cache_path, &CacheKey::TAux.to_string());
+    if Path::new(&t_aux_path).exists() {
+        log::warn!(


Everything looks good to me. I'm just wondering if this should fallback to bincode::deserialize() rather than a warning, or at least warn/fail if the taux on disk disagrees with the default taux values? But maybe that breaks something that I'm unaware of, and there's a good reason why this is just a warning. Also, this may not even be an issue if the fixed-rows-to-discard feature is off by default.

The idea is to fail early. If there is a t_aux file, then something with your setup/environment is wrong and you should really fix it. An implicit fallback could lead to hard to debug errors.

If someone uses this code to do a sector update on a previously sealed sector, why fail if t_aux exists?

We don't fail, we just warn that it is ignored. Of course things then might fail further down the line, hence the warning.

When we talked about it, I though we wanted a clear separating between "you run it with t_aux" and "you run it without t_aux". The "no t_aux" code path, never ever touches that file. In case there is a t_aux file, I would expect that it's more often accidental, rather then intentional.

But if we want to blur that distinction, I can make it read in the t_aux file. Though I'd prefer not doing that.

When we talked about it, I though we wanted a clear separating between "you run it with t_aux" and "you run it without t_aux". The "no t_aux" code path, never ever touches that file. In case there is a t_aux file, I would expect that it's more often accidental, rather then intentional.

Since the case of the 'no t_aux' doesn't touch the file, it shouldn't care that it's present. Warning that it's present and unused is ok, but any failure caused by it being present will be a problem considering that just about every sealed and updated sector that exists today has it present. There's no world where people can run this and be expected to re-seal everything, or manually remove every t_aux on disk in their operation to get the benefit. Sure, someone may choose to start an operation with it enabled and that's the only legitimate case where 'no t_aux' files exist on disk.

I was thinking that falling back to bincode::deserialize() would make the fixed-rows-to-discard feature mean something like "for all sectors going forward seal using the default value for rows_to_discard, but for all previously sealed sectors (e.g. during sector updates) use that sector's t_aux file (if it exists)".

Alright, I've added the fallback. I squashed the commits as I wanted to change the commit to reflect the reality. The diff between before and after the force push is pretty clean though: https://github.com/filecoin-project/rust-fil-proofs/compare/2ee6eda8e577728a3f8dcf3e732859e1442ec201..b02359f97effeca0f84ff95543284527b04975d4

Thanks for making the change. Everything looks good to me

filecoin-proofs/src/api/update.rs

If the `fixed-rows-to-discard` feature is enabled, then TreeR has a fixed number of rows discarded. There is no option to change that value. This also means that we no longer need to persist the `TemporaryAux` file `t_aux`. Tough if such a file exists, it's used.

filecoin-proofs/src/api/util.rs

cryptonemo · 2023-10-27T12:38:02Z

filecoin-proofs/src/api/seal.rs

@@ -299,7 +301,8 @@ where

    // Persist p_aux and t_aux here
    util::persist_p_aux::<Tree>(&p_aux, cache_path.as_ref())?;
-    util::persist_t_aux::<Tree>(&t_aux, cache_path.as_ref())?;
+    #[cfg(not(feature = "fixed-rows-to-discard"))]


Last nit on this -- could we instead have a persist_t_aux defined for this feature config that is more or less an inline'd no-op? I think the switching at this level is still not optimal because it's not clear that fixed-rows-to-discard means we don't need t_aux, although this is an initial case where that's true. It also consolidates all switching on this config related to the p_aux/t_aux to the util methods.

I think the switching at this level is still not optimal because it's not clear that fixed-rows-to-discard means we don't need t_au

I think of it as the reverse. If I would read the code and see a persist_t_aux function call, i would think "oh, t_aux is always persisted". Only if I then look at the implementation I see that it's actually not persisted in some cases. In the current code I see it right away that if that feature is enabled, nothing will be persisted. I would even see that there isn't even a persist_t_aux implementation at all.

Yes, I saw that. I still think localizing it keeps everything consistent

I'd prefer clarity over consistency. Also I'm not sure if it would be more consistent. It's really two cases. One case is "there's a different implementation with and without the feature" and the other case is "only if the feature is not set there's an implementation".

cryptonemo · 2023-10-27T12:44:05Z

filecoin-proofs/src/api/util.rs

+            "`t_aux` file exists, use that file instead of the default values. t_aux={:?}",
+            &t_aux_path
+        );
+        read_t_aux_file(cache_path)


Is this what we wanted here? I thought if it existed with the config option set, it would warn loudly that it exists and is being ignored and then continue with the generated default?

That's how I understood you and @DrPeterVanNostrand. If we would continue with the default values (and the values in t_aux were different) then things might fail at later stages. I thought that's what you wanted to prevent.

I could see the case for either -- but if we're going with the 'supraseal will never touch t_aux` idea, then I think what I said is consistent. If it's better to support what's there, I'm ok with that, but it's a different model

That's how I understood you and @DrPeterVanNostrand. If we would continue with the default values (and the values in t_aux were different) then things might fail at later stages. I thought that's what you wanted to prevent.

I'm ok with it -- it's just different than the 'fixed-rows-to-discard requiring the defaults only and ignoring t_aux' model we also talked about. I don't know yet which has the bigger potential for breakage 🤔

I would prefer the "supraseal will never touch t_aux" idea. But wasn't it then previously doing what you now describe? If I look at

rust-fil-proofs/filecoin-proofs/src/api/util.rs

Lines 101 to 106 in 2ee6eda

if Path::new(&t_aux_path).exists() {

log::warn!(

"`t_aux` file exists, but is ignored t_aux={:?}",

&t_aux_path

);

}

(that's the old version), there is a warning that the file exists, but then it keeps on going with the defaults.

What I would prefer (but I can see the case of not doing that), would be to error in case the file exists, to indicate that you should enable this feature only on new sectors.

Ok, I was confused on the old version then -- I thought it would error if t_aux existed, which is the worst option since it always exists for current sectors. I think I said I'd prefer it to warn loudly that it was being ignored and continue with the defaults in that case.

That said, I can now see the case for warning, but then using the existing (i.e. as it is now). It's a fine point, but we need (documented) clarity either way. Maybe I'm leaning toward where it is now (using existing t_aux if found) because it has a better chance of working on installs where a non-standard rows_to_discard was used on the existing sealed sectors (and going forward with supraseal, it would start using the default) 🤔

filecoin-proofs/src/api/util.rs

storage-proofs-core/src/settings.rs

vmx requested review from cryptonemo and DrPeterVanNostrand as code owners September 14, 2023 16:17

vmx mentioned this pull request Sep 14, 2023

feat: add fallback option if no t_aux file exists #1715

Closed

cryptonemo reviewed Sep 14, 2023

View reviewed changes

filecoin-proofs/src/api/mod.rs Outdated Show resolved Hide resolved

DrPeterVanNostrand reviewed Sep 15, 2023

View reviewed changes

filecoin-proofs/src/api/mod.rs Outdated Show resolved Hide resolved

vmx marked this pull request as draft September 19, 2023 13:03

vmx force-pushed the fixed-rows-to-discard branch from 644d6a5 to fd0541a Compare September 26, 2023 14:12

vmx force-pushed the fixed-rows-to-discard branch from fd0541a to d5cd8ca Compare October 24, 2023 10:37

vmx marked this pull request as ready for review October 24, 2023 10:39

DrPeterVanNostrand reviewed Oct 24, 2023

View reviewed changes

cryptonemo reviewed Oct 24, 2023

View reviewed changes

filecoin-proofs/src/api/update.rs Outdated Show resolved Hide resolved

vmx force-pushed the fixed-rows-to-discard branch from 2ee6eda to b02359f Compare October 26, 2023 13:25

DrPeterVanNostrand reviewed Oct 26, 2023

View reviewed changes

filecoin-proofs/src/api/util.rs Show resolved Hide resolved

DrPeterVanNostrand reviewed Oct 26, 2023

View reviewed changes

filecoin-proofs/src/api/util.rs Show resolved Hide resolved

cryptonemo reviewed Oct 27, 2023

View reviewed changes

filecoin-proofs/src/api/util.rs Outdated Show resolved Hide resolved

cryptonemo reviewed Oct 27, 2023

View reviewed changes

filecoin-proofs/src/api/util.rs Outdated Show resolved Hide resolved

cryptonemo reviewed Oct 27, 2023

View reviewed changes

storage-proofs-core/src/settings.rs Show resolved Hide resolved

chore: use constant instead of magic number

49c5e08

cryptonemo approved these changes Oct 27, 2023

View reviewed changes

vmx merged commit 89ca815 into master Oct 27, 2023
32 checks passed

vmx deleted the fixed-rows-to-discard branch October 27, 2023 17:23

rjan90 mentioned this pull request Nov 6, 2023

Supranational brownfield PC2 <> Lotus-Miner filecoin-project/lotus#10983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow sealing without t_aux files #1717

Allow sealing without t_aux files #1717

vmx commented Sep 14, 2023

vmx commented Sep 19, 2023

vmx commented Sep 20, 2023

vmx commented Oct 24, 2023

DrPeterVanNostrand Oct 24, 2023 •

edited

Loading

vmx Oct 25, 2023

cryptonemo Oct 25, 2023

vmx Oct 25, 2023

cryptonemo Oct 25, 2023

DrPeterVanNostrand Oct 25, 2023

vmx Oct 26, 2023

DrPeterVanNostrand Oct 26, 2023

cryptonemo Oct 27, 2023

vmx Oct 27, 2023

cryptonemo Oct 27, 2023

vmx Oct 27, 2023

cryptonemo Oct 27, 2023

vmx Oct 27, 2023

cryptonemo Oct 27, 2023

cryptonemo Oct 27, 2023

vmx Oct 27, 2023

cryptonemo Oct 27, 2023

	if Path::new(&t_aux_path).exists() {
	log::warn!(
	"`t_aux` file exists, but is ignored t_aux={:?}",
	&t_aux_path
	);
	}

Allow sealing without t_aux files #1717

Allow sealing without t_aux files #1717

Conversation

vmx commented Sep 14, 2023

vmx commented Sep 19, 2023

vmx commented Sep 20, 2023

vmx commented Oct 24, 2023

DrPeterVanNostrand Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrPeterVanNostrand Oct 24, 2023 •

edited

Loading