CHIA-3856 Use an adapted version of deficit round robin algorithm in TransactionQueue's pop #20351
Conversation
AmineKhaldi
added
the
Changed
github-actions
bot
added
the
merge_conflict
Pull Request Test Coverage Report for Build 20236441715Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
arvidn
left a comment
arvidn
left a comment
There was a problem hiding this comment.
My understanding of your implementation is that it:
- scans for a transaction whose cost >= the peer's deficit counter
1.2 If found, ingest transaction and decrement the deficit counter by the cost - If not found, increment the deficit counter of all peers by the lowest cost transaction, and goto 1
I don't think you need the cursor for fairness anymore, you'll get fairness anyway, since you track the deficit counters.
I think it could be made a bit more efficient by reducing the linear scan into a heap pop. It may introduce some more complexity though. This is not a complete thought. But imagine if every peer had a sort "key" which was its deficit_counter - cost_of_top_tx. You could have a priority queue (or really, a heap would suffice) of those peers. Every time a deficit counter is adjusted, the peer is resorted. You always pop from the top (which is O(1)). You'd need to figure out when to increment the deficit counters and by how much. So maybe this wouldn't be so much simpler.
| log: logging.Logger | ||
| _max_tx_clvm_cost: uint64 | ||
| # Map of peer ID to deficit counter in the context of deficit round robin | ||
| _deficit_counters: dict[bytes32, int] |
There was a problem hiding this comment.
isn't _queue_dict also a map of the same peer IDs?
It would seem cheaper and simpler to stick this int in that dict instead. Am I missing something?
| self._index_to_peer_map = new_peer_map | ||
| if result is not None: | ||
| return result | ||
| # Map of peer ID to its top transaction's advertised cost |
There was a problem hiding this comment.
I think this deserves a comment explaining the idea behind this behavior. that we want to service transactions fairly between peers, based on cost.
| num_peers = len(self._index_to_peer_map) | ||
| if num_peers == 0: | ||
| continue | ||
| start = self._list_cursor |
There was a problem hiding this comment.
I don't think you need the _list_cursor anymore, now that you look at every peer every time you pop. There's a risk it might not be very efficient though.
It might be OK for a first version. A heap might be a more efficient choice
|
|
||
| _list_cursor: int # this is which index | ||
| _queue_length: asyncio.Semaphore | ||
| _index_to_peer_map: list[bytes32] |
There was a problem hiding this comment.
I don't think you need _index_to_peer_map anymore, just like you don''t need _list_cursor anymore. You visit every peer every time you pop(), so there's no need to keep any cursor position in between calls to pop().
| if tx_info is None: | ||
| top_tx_advertised_cost = max( | ||
| (t.advertised_cost for t in entry.peers_with_tx.values()), default=self._max_tx_clvm_cost | ||
| ) | ||
| else: | ||
| top_tx_advertised_cost = tx_info.advertised_cost |
There was a problem hiding this comment.
I think this case warrants a comment. This is where we don't know the cost (or fee) for a transaction, so we want to assume a very high cost. I don't think we should search peer_with_tx, we should just assume it has a really high cost. This is just for backwards compatibility, right?
| if tx_info is None: | |
| top_tx_advertised_cost = max( | |
| (t.advertised_cost for t in entry.peers_with_tx.values()), default=self._max_tx_clvm_cost | |
| ) | |
| else: | |
| top_tx_advertised_cost = tx_info.advertised_cost | |
| top_tx_advertised_cost = self._max_tx_clvm_cost if tx_info is None else tx_info.advertised_cost |
2 participants
Purpose:
Converts
TransactionQueue'spopfrom a simple round robin across peers to a deficit round robin.Current Behavior:
TransactionQueue'spopimplements a simple round robin across peers.New Behavior:
TransactionQueue'spopimplements an adapted version of the deficit round robin algorithm.Testing Notes: