refactor: resolve BigInt3 structure

[MoigeMatino]

What does this PR do?

• Resolves BigInt3 structure

Related Issues?

• Resolves Resolve as BigInt3 structure #346

[MaksymMalicki]

The code looks good, I have run the CI and lint and unit tests have failed. Make sure to run make unit in the project and fix the failing code. Also run golangci-lint run and gofmt -w . to lint the code

[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 hi,
left 2 comments in your code.

So for now, you extracted the logic to create a helper function, in order to avoid code duplication. But you dont use this function. You should use it where it is needed in the codebase!

[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 left a minor comment, otherwise lgtm !

[TAdev0]

lgtm!

[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!