refactor: resolve BigInt3 structure

[MoigeMatino]

What does this PR do?

• Resolves BigInt3 structure

Related Issues?

• Resolves #346

[MoigeMatino]

@MaksymMalicki the tests are failing due to a circular import issue emanating from the hinter package that the secp_utils.go relies on. I'm guessing the easiest solution would be to decouple the functionality by having it in a separate package from hintrunner? Any idea on how i can resolve this? All help will be appreciated. @cicr99 @har777

[MaksymMalicki]

Hi @MoigeMatino, thank you for the contribution. I understand the problem and agree, that this functionality needs to be decoupled. However I don't think that creating a separate utility file just for BigInt3 is a good idea. I think it would be a better idea, to place this new utility where the problematic GetConsecutiveValues() functionality is.

Edit: On the second thought, the placement of the GetConsecutiveValues functionality is also wrong in my opinion. It should be placed inside the memory package and modified to look something like this:

func (memory *Memory) GetConsecutiveValues(addr MemoryAddress, size int16) ([]MemoryValue, error) {
	values := make([]MemoryValue, size)
	for i := int16(0); i < size; i++ {
		nAddr, err := addr.AddOffset(i)
		if err != nil {
			return nil, err
		}
		v, err := memory.ReadFromAddress(&nAddr)
		if err != nil {
			return nil, err
		}
		values[i] = v
	}
	return values, nil
}

The problem should then solve itself, as the problematic functionality will be removed from the hinter package. @cicr99 thoughts?

[cicr99]

@MoigeMatino I agree with @MaksymMalicki on the second approach. Functionalities such as GetConsecutiveValues, WriteToNthStructField and WriteUint256ToAddress should not go in the operand.go file. The memory package seems like a fine place to put it, in the memory.go file I'd say. @MoigeMatino could you move them? I don't think it will involve much modification of the code, but if it does we can solve this in a separate PR

[MoigeMatino]

Hi @MoigeMatino, thank you for the contribution. I understand the problem and agree, that this functionality needs to be decoupled. However I don't think that creating a separate utility file just for BigInt3 is a good idea. I think it would be a better idea, to place this new utility where the problematic GetConsecutiveValues() functionality is.

Edit: On the second thought, the placement of the GetConsecutiveValues functionality is also wrong in my opinion. It should be placed inside the memory package and modified to look something like this:

func (memory *Memory) GetConsecutiveValues(addr MemoryAddress, size int16) ([]MemoryValue, error) {
	values := make([]MemoryValue, size)
	for i := int16(0); i < size; i++ {
		nAddr, err := addr.AddOffset(i)
		if err != nil {
			return nil, err
		}
		v, err := memory.ReadFromAddress(&nAddr)
		if err != nil {
			return nil, err
		}
		values[i] = v
	}
	return values, nil
}

The problem should then solve itself, as the problematic functionality will be removed from the hinter package. @cicr99 thoughts?

@MaksymMalicki Thanks for your response, this is well noted 👍🏽

[MoigeMatino]

@MoigeMatino I agree with @MaksymMalicki on the second approach. Functionalities such as GetConsecutiveValues, WriteToNthStructField and WriteUint256ToAddress should not go in the operand.go file. The memory package seems like a fine place to put it, in the memory.go file I'd say. @MoigeMatino could you move them? I don't think it will involve much modification of the code, but if it does we can solve this in a separate PR

@cicr99 Thanks for the response, on it 🚀

[MoigeMatino]

@cicr99 @MaksymMalicki I think creating a separate PR for moving GetConsecutiveValues, WriteToNthStructField and WriteUint256ToAddress to memory.go would be ideal, I've noted that it needs a number of changes on multiple files. I think, i'd ideally need to close that PR first then work on this one because the BigInt3 structure resolution depends on GetConsecutiveValues. Happy to receive further guidance on this.

[MaksymMalicki]

Sure thing @MoigeMatino, let us know when the new PR is ready! Good luck!

[MoigeMatino]

Sure thing @MoigeMatino, let us know when the new PR is ready! Good luck!

Will do, thanks @MaksymMalicki

[cicr99]

@MoigeMatino I've added the label blocked to this draft until the problem with the dependencies is solved. I've also created a separate issue for it #384. Are you currently working on that? I can assign you to it and move the task to "In Progress" if you are, just let us know

[MoigeMatino]

@MoigeMatino I've added the label blocked to this draft until the problem with the dependencies is solved. I've also created a separate issue for it #384. Are you currently working on that? I can assign you to it and move the task to "In Progress" if you are, just let us know

@cicr99 that's fine and yes I'm currently working on issue #384.

[cicr99]

@MoigeMatino that's great! Could you comment on the issue #384 so we can assign you to it?

[MoigeMatino]

@MoigeMatino that's great! Could you comment on the issue #384 so we can assign you to it?

@cicr99 Done. Apologies that #384 has taken a while, closing on this ASAP.

[TAdev0]

@MoigeMatino now the problem with dependencies has been solved, you should be able to work on this issue again

[TAdev0]

@MoigeMatino any update?

[MoigeMatino]

@MoigeMatino any update?

@TAdev0 resolving a couple of issues, should be done soon. Will reach out in case of anything.

[MoigeMatino]

@TAdev0 This PR is now ready for review.

[TAdev0]

@MoigeMatino just saw your last commit. You still don't use the newly created helper function. Do you need any help? you need to remove the comments you added as well

[MoigeMatino]

@MoigeMatino just saw your last commit. You still don't use the newly created helper function. Do you need any help? you need to remove the comments you added as well

@TAdev0 I've identified the place to use ResolveAsBigInt3 i.e in zerohint_signature.go. I've had to alter the function signature for ResolveAsBigInt3, newVerifyZeroHint and createVerifyZeroHinter.

However, now I'm encountering an error relating to the invocation of func MemoryValueFromFieldElement. I'd very much appreciate help with that, pushing the changes just so you can see.

Notably also, changes to func createVerifyZeroHinter signature result in changes to other modules as well, not sure if that's the expected behaviour especially for the functions that call createVerifyZeroHinter. Clarity on this will be appreciated.

[TAdev0]

@MoigeMatino hey,

I just tested locally and it shouldn't be that difficult.

I copy pasted your code in memory_value.go file :

func (memory *Memory) ResolveAsBigInt3(valAddr MemoryAddress) ([3]*f.Element, error) {
	valMemoryValues, err := memory.GetConsecutiveMemoryValues(valAddr, int16(3))
	if err != nil {
		return [3]*f.Element{}, err
	}

	var valValues [3]*f.Element
	for i := 0; i < 3; i++ {
		valValue, err := valMemoryValues[i].FieldElement()
		if err != nil {
			return [3]*f.Element{}, err
		}
		valValues[i] = valValue
	}

	return valValues, nil
}

Then, i identified that this logic is used in zerohint_signatures.go AND zerohint_ec.go

For example, in zerohint_signatures , line 45 :

			valMemoryValues, err := vm.Memory.GetConsecutiveMemoryValues(valAddr, int16(3))
			if err != nil {
				return err
			}

			// [d0, d1, d2]
			var valValues [3]*fp.Element

			for i := 0; i < 3; i++ {
				valValue, err := valMemoryValues[i].FieldElement()
				if err != nil {
					return err
				}
				valValues[i] = valValue
			}

can easily by replaced by :

	valValues, err := vm.Memory.ResolveAsBigInt3(valAddr)
			if err != nil {
				return err
			}

you should do the same in other places where you can do it, in signatures and ec hint files

[TAdev0]

@MoigeMatino ResolveAsBigInt3 should be used :

• 4 times in signatures hint
• a few times (around 10) in EC hints without additional modification either. For some, we cannot use it, or we could but with additional modifications

[TAdev0]

@MoigeMatino any update on this?

[MoigeMatino]

@MoigeMatino any update on this?

@TAdev0 will be finishing on this from tomorrow, been having a busy week 😮‍💨

[MoigeMatino]

Hi @TAdev0 👋🏾. I've pushed the changes. Must admit it took me having to study the code a bit more to properly address the changes that needed to be made. It was a fun weekend, I must say 🥳

[TAdev0]

@MoigeMatino as Hari pointed out, please read the file entirely, y points are extracted in multiple places in the EC hint file

[MoigeMatino]

@MoigeMatino as Hari pointed out, please read the file entirely, y points are extracted in multiple places in the EC hint file

@TAdev0 This was an oversight on my part🙈, resolved the parsing of all y values in the EC hint file, changes pushed.

[TAdev0]

looks good!