Fullnode related changes from wallet protocol and unharded derivation #8522
Conversation
| @@ -312,16 +317,18 @@ async def _reconsider_peak( | |||
| tx_removals, tx_additions = [], [] | |||
| if block.is_transaction_block(): | |||
| assert block.foliage_transaction_block is not None | |||
| await self.coin_store.new_block( | |||
| added, _ = await self.coin_store.new_block( | |||
There was a problem hiding this comment.
@Yostra This looks like a bug. new_block() returns (removed, added). here you're binding the removed records to a variable called added.
There need to be a test that exercises this and fail as this code is written now. Otherwise we should remove this code to not return these lists at all (since clearly they aren't being used)
There was a problem hiding this comment.
I agree. Changing the blockchain logic is pretty risky as well, and this can be done externally. We can read the coinstore from outside of this file.
There was a problem hiding this comment.
Those lists are used only to notify wallet of the changes, in this case (that was broken) change happens when we add a genesis block and in practice this wouldn't happen as no wallet can connect before blockchain is initialized. I'll fix a typo.
There was a problem hiding this comment.
I think, instead of returning added and remived CoinRecords, we should just return "changed coin IDs". and anything the wallet is subscribed to, we can lookup via the CoinStore. Presumably most coins are not subscribed to, and it would be a net improvement. That way, we also wouldn't need new_block() to return anything, since we already have the coin IDs added and removed at the call site.
There was a problem hiding this comment.
(I'd be happy to propose that patch when I'm done with what I'm working on right now)
| """ | ||
| When a new block is added, this is called, to check if the new block is the new peak of the chain. | ||
| This also handles reorgs by reverting blocks which are not in the heaviest chain. | ||
| It returns the height of the fork between the previous chain and the new chain, or returns | ||
| None if there was no update to the heaviest chain. | ||
| """ | ||
| peak = self.get_peak() | ||
| lastest_coin_state: Dict[bytes32, CoinRecord] = {} |
There was a problem hiding this comment.
this should probably be renamed to last_coin_state.
There was a problem hiding this comment.
that was supposed to be latest but last works just as well.
| @@ -87,7 +87,7 @@ async def check_spend_bundle_validity( | |||
| ) | |||
| newest_block = additional_blocks[-1] | |||
|
|
|||
| received_block_result, err, fork_height = await blockchain.receive_block(newest_block) | |||
| received_block_result, err, fork_height, coin_changes = await blockchain.receive_block(newest_block) | |||
There was a problem hiding this comment.
this test doesn't seem to verify that coin_changes is correct
There was a problem hiding this comment.
receive_block() is called from many places in the tests. I didn't add verification to those, but had to add , coin_changes or it would fail to unpack the return value.
| @@ -825,13 +858,41 @@ def get_peers_with_peak(self, peak_hash: bytes32) -> List: | |||
| peers_with_peak: List = [c for c in self.server.all_connections.values() if c.peer_node_id in peer_ids] | |||
| return peers_with_peak | |||
|
|
|||
| async def update_wallets( | |||
| self, height: uint32, fork_height: uint32, peak_hash: bytes32, state_update: List[CoinRecord] | |||
There was a problem hiding this comment.
I really think state_update should be a list of coin names, rather than full CoinRecords. Right now we're doing database lookups for every coin being removed (twice, once to validate it and then once in CoinStore) just to be able to pass it in here. In this function we really only care about a few coins (most likely a small subset). If we would defer looking up the coin records, we would presumably save a lot of database I/O.
There was a problem hiding this comment.
an even more efficient way of doing it would be to have the protocol indicate coins as being spent (and maybe at what height) and just pass on the coin IDs to the wallet. Presumably the wallet will be able to look up its coins by ID and set the spent-bit itself.
There was a problem hiding this comment.
I don't think I added second lookup, it was there when _set_spent is called, I just returned the already looked up value.
There was a problem hiding this comment.
But I agree that can probably be optimized so there is only one lookup
There was a problem hiding this comment.
This is the object that wallet receives
@streamable
class CoinState(Streamable):
coin: Coin
spent_height: Optional[uint32]
created_height: Optional[uint32]
There was a problem hiding this comment.
I have a PR where I'm eliminating that lookup, that's why I'm pointing it out. I think it would make sense to make this lookup only for the coins there are subscriptions on.
There was a problem hiding this comment.
Subscriptions can be for both puzzle_hash and coin_id. If we return just a coin id from this we'll miss spend updates for puzzle_hashes.
| @@ -65,7 +65,7 @@ def shutdown(self): | |||
| return | |||
| self.queue.put(("shutdown",)) | |||
| log.info("UPnP, shutting down thread") | |||
| self.thread.join() | |||
| self.thread.join(5) | |||
There was a problem hiding this comment.
this seems like an unrelated change. Is there a good reason to do this? I would expect a comment in the code to explain why we need to detach the thread after 5 seconds
There was a problem hiding this comment.
yes, it's unrelated. I noticed on shutdown node was forever hanging on thread.join() there.
There was a problem hiding this comment.
we should probably fix the issue causing the UPnP thread to hang instead. This will just hide it
| @@ -24,3 +26,13 @@ class CoinRecord(Streamable): | |||
| @property | |||
| def name(self) -> bytes32: | |||
| return self.coin.name() | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
I think it's risky to make this a property. It makes it hard to tell that this call has a cost. Since it's a new member function, there shouldn't be any existing code that requires this to be a property.
…Chia-Network#8522) * fullnode related changes from wallet protocol and unharded derivation * limit total subscriptions per peer * reset counter on disconnect * dict not a set * check membership * remove unused tests, lint
No description provided.