Batched nodes in SortedTroves

[danielattilasimon]

[Note: we should update the base branch after #85 gets merged]. ✅

This is only the first half of batched interest rate delegation work, however it's big enough that it might be best to review and merge it early.

The doubly linked list is extended with additional links to facilitate quick traversal over contiguous batches:

I also took the opportunity to address the hint recovery issues we had in v1 liquity/dev#600. My philosophy was to optimize for the most likely case of interference, where one of the 2 hints gets moved or removed. Recovery when both hints are messed up is almost guaranteed to fail anyway. With that in mind, I came up with the following strategy:

• If the original insert position was found to be in a terminal position (head or tail, i.e. one of the hints is 0) then we start the recovery from that end of the list. This addresses Implement workaround for SortedTroves edge case dev#659.
• If it's obvious which of the 2 hints is outdated, i.e. because one node's been removed or is now on the wrong side of the list, then we start the recovery from the other, good-looking hint.
• If it's not obvious which hint is outdated, i.e. neither node's been removed and the correct insert position is still somewhere between both, then we start looking inwards from both hints simultaneously. Assuming the most likely case where only one hint has been interfered with, we will quickly find the correct position. Previously, we had a 50-50% chance of either recovering quickly or potentially failing.

[bingen]

This is pretty cool!!
I’ll finish reviewing tomorrow, but I’m leaving a first batch (pun intended) of comments.
At high level, my main concern would be that we are writing the batch address for every node, which seems a lot of repeated used gas. But as long as @ColinPlatt doesn’t notice it’s fine. ;-P
More seriously, I think if we can combine exists and batchId it wouldn’t matter, as the storage slot would be used anyway.

[ColinPlatt]

I will fight you

…

On Thu, Apr 4, 2024, 19:18 ßingen ***@***.***> wrote: ***@***.**** commented on this pull request. This is pretty cool!! I’ll finish reviewing tomorrow, but I’m leaving a first batch (pun intended) of comments. At high level, my main concern would be that we are writing the batch address for every node, which seems a lot of repeated used gas. But as long as @ColinPlatt <https://github.com/ColinPlatt> doesn’t notice it’s fine. ;-P More seriously, I think if we can combine exists and batchId it wouldn’t matter, as the storage slot would be used anyway. ------------------------------ In contracts/src/Types/TroveId.sol <#94 (comment)>: > @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.18; + +type TroveId is uint256; Where are we using this? ------------------------------ In contracts/test/AccessControlTest.js <#94 (comment)>: > @@ -402,7 +402,7 @@ contract( ); } catch (err) { assert.include(err.message, "revert"); - assert.include(err.message, " Caller is neither BO nor TroveM"); Nitpick: we can also remove TroveM from test description, and the same below. ------------------------------ In contracts/src/SortedTroves.sol <#94 (comment)>: > ITroveManager public troveManager; // Information for a node in the list struct Node { bool exists; - uint256 nextId; // Id of next node (smaller interest rate) in the list - uint256 prevId; // Id of previous node (larger interest rate) in the list + uint256 nextId; // Id of next node (smaller interest rate) in the list + uint256 prevId; // Id of previous node (larger interest rate) in the list + BatchId batchId; // Id of this node's batch manager, or zero in case of non-batched nodes As currently BatchId is an address, I think we could place the boolean exists at the end to optimize storage. ------------------------------ In contracts/src/SortedTroves.sol <#94 (comment)>: > - function insert (uint256 _id, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) external override { - ITroveManager troveManagerCached = troveManager; + function _insertSlice(ITroveManager _troveManager, uint256 _sliceHead, uint256 _sliceTail, uint256 _annualInterestRate, uint256 _prevId, uint256 _nextId) internal { Why are we receiving TroveManager as a param? Can’t we cache it here inside the function? If I’m not mistaken all the calls to this function are using the storage one directly. ------------------------------ In contracts/src/SortedTroves.sol <#94 (comment)>: > - // Set prev pointer of new head to null - data.nodes[data.head].prevId = 0; - } else if (_id == data.tail) { - // The removed node is the tail - // Set tail to previous node - data.tail = data.nodes[_id].prevId; - // Set next pointer of new tail to null - data.nodes[data.tail].nextId = 0; - } else { - // The removed node is neither the head nor the tail - // Set next pointer of previous node to the next node - data.nodes[data.nodes[_id].prevId].nextId = data.nodes[_id].nextId; - // Set prev pointer of next node to the previous node - data.nodes[data.nodes[_id].nextId].prevId = data.nodes[_id].prevId; - } + // The node being re-inserted can't be a valid hint, use its neighbours instead I don’t fully get this. What’s the expected scenario for this? I mean, why would BorrowOperations send an invalid hint? Wouldn’t it be better to revert to avoid unwanted consequences? ------------------------------ In contracts/src/SortedTroves.sol <#94 (comment)>: > - // The removed node is neither the head nor the tail - // Set next pointer of previous node to the next node - data.nodes[data.nodes[_id].prevId].nextId = data.nodes[_id].nextId; - // Set prev pointer of next node to the previous node - data.nodes[data.nodes[_id].nextId].prevId = data.nodes[_id].prevId; - } + // The node being re-inserted can't be a valid hint, use its neighbours instead + if (_prevId == _id) { + _prevId = nodes[_id].prevId; + } + + if (_nextId == _id) { + _nextId = nodes[_id].nextId; + } + + _removeSlice(_id, _id); Wouldn’t be more efficient to adjust prev/next ids instead of removing and then adding again? Maybe not, as we are hitting the same storage positions so in the end it would compute as non-zero to non-zero write anyway? — Reply to this email directly, view it on GitHub <#94 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACRS32Z6DGH7BHWNTTMVNBLY3WDMVAVCNFSM6AAAAABFXDVPY2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBQGU2DSMJXGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[danielattilasimon]

Gas comparison pre- and post-batching for normal Troves (as delegated Troves aren't implemented in TroveManager/BorrowerOperations yet):

Opening a Trove is slightly more expensive (maybe worth looking into why, as we're talking about non-batched Troves), on the other hand, adjusting interest rate is significantly cheaper now, so it should be worth it in the long run.

Edit: actually, these gas figures aren't reflective of normal usage, as the test cases I'm using to measure them (our Solidity tests) don't use proper hints.

[bingen]

Really cool! All good to me. Some more comments below, the only important one is about the inequality strictness, not sure if I’m missing something.

[danielattilasimon]

I whipped up a test fixture for measuring gas costs of SortedTroves functions. In the case of findInsertPosition(), I'm measuring the costs of finding the correct position when both hints are zero. For all other functions, I'm passing the hints returned by findInsertPosition() (the happy path). Now the gas costs are down in every case: Insert still takes a hit:

Edit: uploaded a new image with corrected gas costs. Initial test had a bug.

Edit 2: and comparing the new SortedTroves (which needs no client-side hacks to find the right hints) against the old one, but using the hacks we employ in the SDK:

It's also a good example of why we had to resort to hacks in v1 😆

[RickGriff]

Awesome batching/slice design and optimization! Just one question about ascending/descending as below.

[RickGriff]

Hmm how can this case occur?

_findInsertPosition should still always return two adjacent nodes (right)? So I guess this case could only occur for a slice length of 2 (and maybe also 1)?

[danielattilasimon]

This happens in case of re-insertion, even if the slice is singular. Of course, BorrowerOperations will try to avoid re-inserting if the interest rate hasn't changed, but it can still happen that the interest rate changes so little that the correct position remains the same.

Because it's a re-insertion, the list will already contain the node that is to be re-inserted at the time that hints are computed off-chain using _findInsertPosition(). In V1, this lead to some issues. The re-insertion was done in 2 steps, by first removing then inserting the node again. As such, the node wasn't part of the list when _validInsertPosition() / _findInsertPosition() was being used on-chain, as opposed to the call to _findInsertPosition() off-chain. This lead to the hints being wrong for re-insertions that didn't change the position of the node. Eventually, we worked around this in the SDK by skipping over the node that was being re-inserted:
https://github.com/liquity/dev/blob/e38edf3dd67e5ca7e38b83bcf32d515f896a7d2f/packages/lib-ethers/src/PopulatableEthersLiquity.ts#L747-L756

When implementing batching, I saw an opportunity to improve on v1 by not removing the node before it's inserted again. This not only lets us keep the hint search simple and free of workarounds, but it also optimizes gas usage in the (rare) case that the position remains the same.

[RickGriff]

This is the case where the batch doesn't yet exist right?

I'm having to remember that batch head/tail values of 0 mean the batch doesn't exist, but nodes[0] is the head/tail of the list. Would it make any sense to make the ID for the special head/tail of the list some magic number other than 0, i.e. uint256.max?

[danielattilasimon]

I can see where you're coming from, it is confusing. I also thought about using a different magic number, but I decided against it because it would break compatibility with existing code where the SortedTroves list is iterated on externally.

However, we could alleviate the confusion by defining the magic ID in a constant, to make its purpose really clear. Since it's both the head and tail of the list, what do you think about calling it ROOT_NODE_ID?

[danielattilasimon]

Made some improvements to readability by pulling out some constants:
464cb76

[danielattilasimon]

Also, I've made sure that the newly written tests still pass if you change ROOT_NODE_ID to e.g. type(uint256).max, just in case we want to change it at some point, but it might brake some tests — haven't checked yet.

[RickGriff]

Hmm should we return _pos here? otherwise this else branch seems redundant (it just returns false, and we throw away the changes to _pos.prevId and _pos.nextId... unless I'm missing something

[danielattilasimon]

Argh, it's pretty confusing when viewed in diff form. I think it's easier to read only the refactored code.

The loop that descends the list:

bold/contracts/src/SortedTroves.sol

Lines 391 to 396 in 464cb76

	function _descendList(ITroveManager _troveManager, uint256 _annualInterestRate, uint256 _startId) internal view returns (uint256, uint256) {
	Position memory pos = Position(_startId, nodes[_startId].nextId);
	while (!_descendOne(_troveManager, _annualInterestRate, pos)) {}
	return (pos.prevId, pos.nextId);
	}

The helper it uses to make a single step:

bold/contracts/src/SortedTroves.sol

Lines 367 to 374 in 464cb76

	function _descendOne(ITroveManager _troveManager, uint256 _annualInterestRate, Position memory _pos) internal view returns (bool found) {
	if (_pos.nextId == ROOT_NODE_ID || _annualInterestRate > _troveManager.getTroveAnnualInterestRate(_pos.nextId)) {
	found = true;
	} else {
	_pos.prevId = _skipToBatchTail(_pos.nextId);
	_pos.nextId = nodes[_pos.prevId].nextId;
	}
	}

In every iteration we evaluate the search condition, and if we're not in the right position yet, take one step (which can be quite a large step, in case we encounter a batch).

If we entered the else branch, the changes aren't wasted, because that position will be tested in the next iteration. If a position meets the search condition, it is returned from the outer loop (_descendList() above).

The reason for factoring out _descendOne() & _ascendOne() and for keeping the positions in memory is to make it easy to implement an alternating walk:

bold/contracts/src/SortedTroves.sol

Lines 420 to 428 in 464cb76

	for (;;) {
	if (_descendOne(_troveManager, _annualInterestRate, descentPos)) {
	return (descentPos.prevId, descentPos.nextId);
	}
	if (_ascendOne(_troveManager, _annualInterestRate, ascentPos)) {
	return (ascentPos.prevId, ascentPos.nextId);
	}
	}

[RickGriff]

Really cool!