Use SHA256 checksums for texture filenames

[catfromplan9]

This matches the behaviour of textures.minecraft.net. Using BLAKE3 checksums for filenames may cause problems in some modded environments that rely on the filename being a matching SHA256 checksum, so it would be wise to copy Mojang here.

[catfromplan9]

Have already tested that this works and causes no issues when upgrading, though old textures will not have the sha256 checksum filename it's probably fine.

[evan-goode]

This matches the behaviour of textures.minecraft.net.

That's a decent enough reason... but changing this now would mean that for existing instances, a skin filename is either the BLAKE3 or the SHA256 checksum of the skin file. It's an internal detail that users shouldn't care about, but it might confuse anyone who happens to take a peek.

Are you trying to solve a problem with a specific mod or are you just planning ahead? I'd say if there are no current known issues, we should delay this change until one is discovered.

[catfromplan9]

This matches the behaviour of textures.minecraft.net.

That's a decent enough reason... but changing this now would mean that for existing instances, a skin filename is either the BLAKE3 or the SHA256 checksum of the skin file. It's an internal detail that users shouldn't care about, but it might confuse anyone who happens to take a peek.

Are you trying to solve a problem with a specific mod or are you just planning ahead? I'd say if there are no current known issues, we should delay this change until one is discovered.

Yes, MCPHackers LaunchWrapper relies on this.

https://github.com/MCPHackers/LaunchWrapper/blob/b1043d0783da530bb1f42d714810555054df40e2/src/main/java/org/mcphackers/launchwrapper/protocol/skin/MojangSkinProvider.java#L201-L203

Maybe you could write a commit that will on upgrade migrate all files to use sha256 filenames?

[catfromplan9]

Maybe you could write a commit that will on upgrade migrate all files to use sha256 filenames?

Or, could you provide a link to a commit that will trigger some sort of migration code on an upgrade? I am willing to write this myself, I just don't know how to go about doing that.

[catfromplan9]

Also this may break vanilla skin caching, but I'm not sure.

[evan-goode]

Yeah, I'd rather rip the band-aid off and move all skins to SHA256 to be consistent with Mojang. And do a major version bump to 4.0.0 since this will break skin URLs.

Maybe you could write a commit that will on upgrade migrate all files to use sha256 filenames?

Sure, I can take a stab at it. We can do it during a database migration and handle it similar to how we do skin deletions:

drasl/player.go

Lines 309 to 320 in 114c929

	// Skin and cape updates are done as follows:
	// 1. Validate with ValidateSkin/ValidateCape
	// 2. Read the texture into memory and hash it with ReadTexture
	// 3. Update the database
	// 4. If the database updated successfully:
	// - Acquire a lock to the texture file
	// - If the texture file doesn't exist, write it to disk
	// - Delete the old texture if it's unused
	//
	// Any update should happen first to the DB, then to the filesystem. We
	// don't attempt to roll back changes to the DB if we fail to write to
	// the filesystem.

To be more resilient against filesystem errors, we could hardlink the sha256.pngs to the blake3.pngs, then update the database, then remove the blake3.pngs. But maybe that's overkill; we'd have to fall back to renaming or copying anyway if the filesystem doesn't support hardlinks.

https://github.com/MCPHackers/LaunchWrapper/blob/b1043d0783da530bb1f42d714810555054df40e2/src/main/java/org/mcphackers/launchwrapper/protocol/skin/MojangSkinProvider.java#L201-L203

From a quick look this might just be an identifier used for caching and might work with any unique string.

Either way, if LaunchWrapper wants to support authlib-injector skin providers, they should not rely on the URL containing the texture SHA256. The authlib-injector spec is explicit that the hash doesn't need to be SHA256: https://github-com.translate.goog/yushijinhun/authlib-injector/wiki/Yggdrasil-%E6%9C%8D%E5%8A%A1%E7%AB%AF%E6%8A%80%E6%9C%AF%E8%A7%84%E8%8C%83?_x_tr_sl=auto&_x_tr_tl=en&_x_tr_hl=en-US#%E6%9D%90%E8%B4%A8-url-%E8%A7%84%E8%8C%83

[catfromplan9]

Yeah, I'd rather rip the band-aid off and move all skins to SHA256 to be consistent with Mojang. And do a major version bump to 4.0.0 since this will break skin URLs.

Maybe you could write a commit that will on upgrade migrate all files to use sha256 filenames?

Sure, I can take a stab at it. We can do it during a database migration and handle it similar to how we do skin deletions:

drasl/player.go

Lines 309 to 320 in 114c929

	// Skin and cape updates are done as follows:
	// 1. Validate with ValidateSkin/ValidateCape
	// 2. Read the texture into memory and hash it with ReadTexture
	// 3. Update the database
	// 4. If the database updated successfully:
	// - Acquire a lock to the texture file
	// - If the texture file doesn't exist, write it to disk
	// - Delete the old texture if it's unused
	//
	// Any update should happen first to the DB, then to the filesystem. We
	// don't attempt to roll back changes to the DB if we fail to write to
	// the filesystem.

To be more resilient against filesystem errors, we could hardlink the sha256.pngs to the blake3.pngs, then update the database, then remove the blake3.pngs. But maybe that's overkill; we'd have to fall back to renaming or copying anyway if the filesystem doesn't support hardlinks.

https://github.com/MCPHackers/LaunchWrapper/blob/b1043d0783da530bb1f42d714810555054df40e2/src/main/java/org/mcphackers/launchwrapper/protocol/skin/MojangSkinProvider.java#L201-L203

From a quick look this might just be an identifier used for caching and might work with any unique string.

Either way, if LaunchWrapper wants to support authlib-injector skin providers, they should not rely on the URL containing the texture SHA256. The authlib-injector spec is explicit that the hash doesn't need to be SHA256: https://github-com.translate.goog/yushijinhun/authlib-injector/wiki/Yggdrasil-%E6%9C%8D%E5%8A%A1%E7%AB%AF%E6%8A%80%E6%9C%AF%E8%A7%84%E8%8C%83?_x_tr_sl=auto&_x_tr_tl=en&_x_tr_hl=en-US#%E6%9D%90%E8%B4%A8-url-%E8%A7%84%E8%8C%83

Even if it does work, I'd rather we follow mojang here. I already PR"d launchwrapper to fix texture URL being hardcoded, I could do another PR to do proper hashing if necessary.

Yeah, adding this to 4.0.0 seems like a good idea. I think my classic protocol PR would also be good for 4.0.0 since it is a major change as well