fix: client merging concurrent calls that shouldn't be merged

[Farber98]

Description

This PR fixes an issue in our Solana client where certain RPC calls could be incorrectly merged by the singleflight.Group mechanism. Previously, if different calls had identical method names but varied in arguments (e.g., different commitments or slot numbers), singleflight would treat them as the same request and return the same response to all callers.

To address this

We are now incorporating request-specific parameters into the deduplication keys. This ensures that truly distinct calls (e.g., different commitments or different slot numbers) are not mistakenly merged into a single request.

https://smartcontract-it.atlassian.net/browse/NONEVM-1161

[DylanTinianov]

NIT:

Suggested change

	var endSlotStr string
	if endSlot == nil {
	endSlotStr = "nil"
	} else {
	endSlotStr = fmt.Sprintf("%d", *endSlot)
	}
	key := fmt.Sprintf("GetBlocks(%d,%s)", startSlot, endSlotStr)
	endSlotStr := "nil"
	if endSlot != nil {
	endSlotStr = fmt.Sprint(*endSlot)
	}
	key := fmt.Sprintf("GetBlocks(%d,%s)", startSlot, endSlotStr)

[Farber98]

much better

[dhaidashenko]

the rest of the call can be replaced with GetBlock(ctx context.Context, slot uint64)

[Farber98]

do you mean replacing:

// Adding slot to the key so concurrent calls to GetBlock for different slots are not merged. Without including the slot,
	// it would treat all GetBlock calls as identical and merge them, returning whichever block it fetched first to all callers.
	key := fmt.Sprintf("GetBlockWithOpts(%d)", slot)
	v, err, _ := c.requestGroup.Do(key, func() (interface{}, error) {
		version := uint64(0) // pull all tx types (legacy + v0)
		return c.rpc.GetBlockWithOpts(ctx, slot, &rpc.GetBlockOpts{
			Commitment:                     c.commitment,
			MaxSupportedTransactionVersion: &version,
		})
	})
	return v.(*rpc.GetBlockResult), err

with

return c.rpc.GetBlock(ctx, slot)

If that's the case I think this is not ideal as we are not providing the optional parameters. Under the hood we would be calling GetBlockWithOpts but not providing the following opts:

&rpc.GetBlockOpts{
	Commitment: c.commitment,
	MaxSupportedTransactionVersion: &version,
}

[dhaidashenko]

Oh, sorry. I meant to leave this comment for the GetLatestBlock. It can look as follows:

func (c *Client) GetLatestBlock(ctx context.Context) (*rpc.GetBlockResult, error) {
	// get latest confirmed slot
	slot, err := c.SlotHeightWithCommitment(ctx, c.commitment)
	if err != nil {
		return nil, fmt.Errorf("GetLatestBlock.SlotHeight: %w", err)
	}

	// get block based on slot
	done := c.latency("latest_block")
	defer done()
	return c.GetBlock(ctx, slot)
}

Instead of

func (c *Client) GetLatestBlock(ctx context.Context) (*rpc.GetBlockResult, error) {
	// get latest confirmed slot
	slot, err := c.SlotHeightWithCommitment(ctx, c.commitment)
	if err != nil {
		return nil, fmt.Errorf("GetLatestBlock.SlotHeight: %w", err)
	}

	// get block based on slot
	done := c.latency("latest_block")
	defer done()
	ctx, cancel := context.WithTimeout(ctx, c.txTimeout)
	defer cancel()
	v, err, _ := c.requestGroup.Do("GetBlockWithOpts", func() (interface{}, error) {
		version := uint64(0) // pull all tx types (legacy + v0)
		return c.rpc.GetBlockWithOpts(ctx, slot, &rpc.GetBlockOpts{
			Commitment:                     c.commitment,
			MaxSupportedTransactionVersion: &version,
		})
	})
	return v.(*rpc.GetBlockResult), err
}

[Farber98]

Nice catch

[dhaidashenko]

It's possible to pass custom opts. We should include them into the key or remove it from the function arguments.
I'm leaning towards second option as the method is only used in one test

[Farber98]

Nice! Removing this

[Farber98]

Had to make some changes in the new multi_client to remove the extra param

[cl-sonarqube-production]

Quality Gate failed

Failed conditions
67.7% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube