chia-blockchain icon indicating copy to clipboard operation
chia-blockchain copied to clipboard

Wallet protocol update

Open Rigidity opened this issue 1 year ago • 17 comments

Purpose:

The wallet protocol is still fairly limited in functionality.

This PR adds a lot of really nice to have features to it, and enables various things wallets could not previously do.

Existing Protocol Limitations:

  • You cannot unsubscribe from puzzle hashes or coin ids without disconnecting from the peer completely, so once you reach the limit you have to reset everything
  • If a puzzle hash or coin id has too many coins to return in one message, the response will be truncated (which is not a reasonable outcome for some use cases)
  • You cannot request coin state without subscribing to the coin id, which is unnecessary and means the subscription limit will be reached faster
  • If a puzzle reveal or solution exceeds the maximum message size when serialized, there is no way to request it
  • It is not currently possible to ask a node for information about the mempool status
  • There is no way to know which transactions belong to your wallet without downloading them all and checking by hand

New Features

  • Removing subscriptions
  • Requesting state without subscribing
  • Listening to the mempool
  • Filtering coin states by amount, hint, and spent status
  • Pagination when syncing coin state for puzzle hashes
  • Mempool fee estimation utilities
  • Request puzzle and solutions with support for backrefs

Proposed New Protocol

Note that transaction_id in the below messages refers to the "spend bundle name", which is calculated by hashing a spend bundle. This is different from a wallet's random or sequential transaction id, and is what is used to identify transactions in the mempool.

Add Puzzle Subscriptions

class RequestAddPuzzleSubscriptions:
    puzzle_hashes: List[bytes32]

class RespondAddPuzzleSubscriptions:
    puzzle_hashes: List[bytes32]
    height: uint32
    header_hash: bytes32

Adds puzzle hashes to the subscription list until the limit has been reached, returning the hashes that were actually added. If the limit is exceeded, the wallet can either bail or subscribe with other nodes instead. Also gives the current peak height and header hash.

Note that unlike RegisterForPhUpdates, this does not return any coin states. There is a RequestPuzzleState message which fetches the current coin states instead. The idea of this is to enable you to subscribe only, request current state only, or do both at the same time.

Add Coin Subscriptions

class RequestAddCoinSubscriptions:
    coin_ids: List[bytes32]

class RespondAddCoinSubscriptions:
    coin_ids: List[bytes32]
    height: uint32
    header_hash: bytes32

Adds coin ids to the subscription list until the limit has been reached, returning the ids that were actually added. If the limit is exceeded, the wallet can either bail or subscribe with other nodes instead. Also gives the current peak height and header hash.

Note that unlike RegisterForCoinUpdates, this does not return any coin states. There is a RequestCoinState message which fetches the current coin states instead. The idea of this is to enable you to subscribe only, request current state only, or do both at the same time.

Add Transaction Subscriptions

class RequestAddTransactionSubscriptions:
    transaction_ids: List[bytes32]

class RespondAddTransactionSubscriptions:
    transaction_ids: List[bytes32]

Adds transaction ids to the subscription list, until the limit has been reached, returning the ids that were actually added. If the limit is exceeded, the wallet can either bail or subscribe with other nodes instead.

This only subscribes to mempool updates, which are not able to be validated since they have not been included in the blockchain. You could receive updates from multiple peers and compare them, determining if you are being spammed or misled by a peer and ban it.

Remove Puzzle Subscriptions

class RequestRemovePuzzleSubscriptions:
    puzzle_hashes: Optional[List[bytes32]]

class RespondRemovePuzzleSubscriptions:
    puzzle_hashes: List[bytes32]

Removes puzzle hashes from the subscription list (or all of them if None), returning the hashes that were actually removed.

Remove Coin Subscriptions

class RequestRemoveCoinSubscriptions:
    coin_ids: Optional[List[bytes32]]

class RespondRemoveCoinSubscriptions:
    coin_ids: List[bytes32]

Removes coin ids from the subscription list (or all of them if None), returning the ids that were actually removed.

Remove Transaction Subscriptions

class RequestRemoveTransactionSubscriptions:
    transaction_ids: Optional[List[bytes32]]

class RespondRemoveTransactionSubscriptions:
    transaction_ids: List[bytes32]

Removes transaction ids from the subscription list (or all of them if None), returning the ids that were actually removed.

Request Puzzle State

class RequestPuzzleState:
    puzzle_hashes: List[bytes32]
    previous_height: uint32
    header_hash: bytes32
    max_height: Optional[uint32]
    filters: CoinStateFilters
    subscribe_when_finished: bool

class RespondPuzzleState:
    puzzle_hashes: List[bytes32]
    height: uint32
    header_hash: bytes32
    is_finished: bool
    coin_states: List[CoinState]

class RejectPuzzleState:
    pass

class CoinStateFilters:
    include_spent: bool
    include_unspent: bool
    include_hinted: bool
    min_amount: uint64

Requests coin states that match the given puzzle hashes (or hints).

Unlike RegisterForPhUpdates, this does not add subscriptions for the puzzle hashes automatically. When subscribe_when_finished is set to True, it will add subscriptions, but only once the last batch has been returned.

As well as this, previously it was impossible to get all coin records if the number of items exceeded the limit. This implementation allows you to continue where you left off with previous_height and header_hash.

If a reorg of relevant blocks occurs while doing so, previous_height will no longer match header_hash. This can be handled by a wallet by simply backtracking a bit, or restarting the download from zero. It could be inconvenient, but at least you can detect it. In the event that a reorg is detected by a node, RejectPuzzleState will be returned. This is the only scenario it will be rejected directly like this.

Additionally, it is now possible to filter out spent, unspent, or hinted coins, as well as coins below a minimum amount. This can reduce the risk of spamming or DoS of a wallet in some cases, and improve performance.

Request Coin State

class RequestCoinState:
    coin_ids: List[bytes32]
    previous_height: uint32
    header_hash: bytes32
    max_height: Optional[uint32]
    subscribe: bool

class RespondCoinState:
    coin_ids: List[bytes32]
    coin_states: List[CoinState]

class RejectCoinState:
    pass

Request coin states that match the given coin ids.

Unlike RegisterForCoinUpdates, this does not add subscriptions for the coin ids automatically. When subscribe is set to True, it will add and return as many coin ids to the subscriptions list as possible.

Unlike the new RequestPuzzleState message, this does not implement batching for simplicity. However, you can still specify the previous_height and header_hash to start from.

If a reorg of relevant blocks has occurred, previous_height will no longer match header_hash. This can be handled by a wallet depending on the use case (for example by restarting from zero). It could be inconvenient, but at least you can detect it. In the event that a reorg is detected by a node, RejectCoinState will be returned. This is the only scenario it will be rejected directly like this.

Transaction Added

class TransactionAddedUpdate:
    transaction_id: bytes32
    height_added: uint32
    cost: uint64
    additions: List[Coin]
    removals: List[Coin]

Sent to wallet peers using the new protocol version when a transaction matching any subscription is added to the mempool.

Transaction Removed

class TransactionRemovedUpdate:
    transaction_id: bytes32
    height_removed: uint32
    inclusion_header_hash: Optional[bytes32]
    additions: List[Coin]
    removals: List[Coin]

Sent to wallet peers using the new protocol version when a transaction matching any subscription is removed from the mempool. The inclusion_header_hash is None when it's removed from the mempool but not included in a block. Otherwise, it's the header hash of height_removed, and the block that the transaction was included in.

Request Puzzle Solution Backrefs

class RequestPuzzleSolutionBackrefs:
    coin_id: bytes32
    height: uint32

class RespondPuzzleSolutionBackrefs:
    puzzle_reveal: Bytes
    solution: Bytes

Unlike RequestPuzzleSolution, this will return the serialized puzzle and solution with backrefs. It's not clear to me how this would work when the program reaches into previous blocks via compression, so more thought is needed.

Request Cost Info

class RequestCostInfo:
    pass

class RespondCostInfo:
    max_block_cost: uint64
    max_mempool_cost: uint64
    current_mempool_cost: uint64
    estimated_total_fee: uint64

This is a rough draft, but essentially the wallet protocol should be able to get information needed to better estimate fees.

Concerns

The data which a full node gives you cannot be trusted unless it is your own node. These additions to the protocol make no effort to make untrusted data any easier to validate. The best method I am aware of is still just to cross reference with other peers and ban outliers.

Checklist (WIP)

Coin Subscriptions & State

  • [x] Waiting for https://github.com/Chia-Network/chia-blockchain/pull/17300 to be merged.
  • [x] New messages for adding and removing subscriptions.
  • [x] New messages for fetching coin state without subscribing.
  • [x] Synced coin states for puzzle hashes can be filtered and sent in batches.
  • [x] Update serialization and regression tests with these messages.
  • [ ] Write unit tests for these subscription and coin state protocol messages.
  • [ ] Write tests for syncing a wallet from zero using this new sync protocol.
  • [ ] Write tests for syncing from the last known height, and recovering when a reorg occurs.
  • [ ] Write tests for a reorg occurring during a wallet's sync.

Transaction Subscriptions

  • [ ] New messages for adding and removing transaction subscriptions.
  • [ ] New message(s) for transaction status updates.
  • [ ] Implement transaction subscriptions, using the local mempool.
  • [ ] Update serialization and regression tests with these messages.
  • [ ] Write unit tests for these subscription protocol messages.
  • [ ] Write tests for what happens when a reorg occurs.

Before Merge

  • [ ] Ensure protocol version is correct, and doesn't need to be bumped again.
  • [ ] Implement protocol messages in Rust https://github.com/Chia-Network/chia_rs/pull/369 to keep parity.

Rigidity avatar Jan 16 '24 19:01 Rigidity

Pull Request Test Coverage Report for Build 7615663424

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report. To ensure accuracy in future PRs, please see these guidelines. A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 90.547%

Totals Coverage Status
Change from base Build 7574540476: -0.05%
Covered Lines: 95132
Relevant Lines: 105023

💛 - Coveralls

coveralls-official[bot] avatar Jan 22 '24 19:01 coveralls-official[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Feb 01 '24 15:02 github-actions[bot]

In general, there are opportunities to improve the protocol design as new messages are added. Subscriptions essentially open a "channel" multiplexed over the message stream, so we could introduce an explicit channel concept - just an incrementing nonce namespaced on each websocket.

The request could allow specifying a start block height, so that we don't sync from genesis each time. Wallets can track a "current height" per puzzle hash.

Perhaps something like:

# non-message struct
class PuzzleSubscriptionRequest:  
  puzzle_hash: bytes32
  height: uint32

# message
class RequestAddPuzzleSubscriptions:
  requests: List[PuzzleSubscriptionRequest]

# non-message struct
class RespondSubscriptionRequest:
  channel: uint32
  puzzle_hash: bytes32  
  height: uint32
  header_hash: bytes32

# message
class RespondAddPuzzleSubscriptions:
  requests: List[RespondSubscriptionRequest]

This makes unsubscribe simpler:

class RequestRemoveSubscriptions:
  channel: List[uint32]

class RespondRemoveSubscriptions:
  channel: List[uint32]

Update messages might look something like:

class SubscriptionUpdate:
  channel: uint32
  updates: list[(info about coin spend)] 
  completed_updated_to_height: uint32

The completed_updated_to_height field is a height that we have completely exhausted all updates for. This might be much larger than the block of the last update (if we see no further spends for example) or ONE LESS than the block the last update occurs in (if there are further updates in the same block that will not fit into this message). Essentially, it's the value the wallet can later subscribe to and be assured to have not missed an update.

richardkiss avatar Feb 03 '24 01:02 richardkiss

Generally speaking, having multiple potential responses to a particular request is annoying. It means when a client makes a request, it has multiple message IDs it needs to wait on to find a response.

The serialization protocol doesn't support enum types, and there's no obvious way to see how to do it. However, we could simply have each different potential response be Optional. As an example, rather than having two messages each with their own id:

class RespondPuzzleState:
    puzzle_hashes: List[bytes32]
    next_height: Optional[uint32]
    next_header_hash: Optional[bytes32]
    coin_states: List[CoinState]

class RejectPuzzleState:
    header_hash: Optional[bytes32]

we could merge them:

# used in `RespondPuzzleState`
class ResponsePuzzleStateOK:
    puzzle_hashes: List[bytes32]
    next_height: Optional[uint32]
    next_header_hash: Optional[bytes32]
    coin_states: List[CoinState]

# used in `RespondPuzzleState`
class ResponsePuzzleStateFail:
    header_hash: Optional[bytes32]

# message
class RespondPuzzleState:
    ok: Optional[ResponsePuzzleStateOK]
    fail: Optional[ResponsePuzzleStateFail]

Here, exactly one of ok and fail should be not None. We can't enforce this through type-checking, but we can enforce at runtime, and simply fail on messages that don't comply like we would any other ill-formed message (for example, disconnect from or otherwise punish the remote).

richardkiss avatar Feb 03 '24 03:02 richardkiss

In general, there are opportunities to improve the protocol design as new messages are added. Subscriptions essentially open a "channel" multiplexed over the message stream, so we could introduce an explicit channel concept - just an incrementing nonce namespaced on each websocket.

The request could allow specifying a start block height, so that we don't sync from genesis each time. Wallets can track a "current height" per puzzle hash.

Perhaps something like:

# non-message struct
class PuzzleSubscriptionRequest:  
  puzzle_hash: bytes32
  height: uint32

# message
class RequestAddPuzzleSubscriptions:
  requests: List[PuzzleSubscriptionRequest]

# non-message struct
class RespondSubscriptionRequest:
  channel: uint32
  puzzle_hash: bytes32  
  height: uint32
  header_hash: bytes32

# message
class RespondAddPuzzleSubscriptions:
  requests: List[RespondSubscriptionRequest]

This makes unsubscribe simpler:

class RequestRemoveSubscriptions:
  channel: List[uint32]

class RespondRemoveSubscriptions:
  channel: List[uint32]

Update messages might look something like:

class SubscriptionUpdate:
  channel: uint32
  updates: list[(info about coin spend)] 
  completed_updated_to_height: uint32

The completed_updated_to_height field is a height that we have completely exhausted all updates for. This might be much larger than the block of the last update (if we see no further spends for example) or ONE LESS than the block the last update occurs in (if there are further updates in the same block that will not fit into this message). Essentially, it's the value the wallet can later subscribe to and be assured to have not missed an update.

I don't understand the benefit of this, it seems like a more complicated API to do roughly the same thing. But maybe I'm misunderstanding something? Subscribing doesn't actually return any coin states, it just registers that you're interested in a given set of puzzle hashes going forward whenever coin states are updated.

The RequestPuzzleState message allows you to specify the height and header hash to start from, like you're describing, and you keep requesting more coin states until you've reached the peak or you decide to stop.

Rigidity avatar Feb 03 '24 04:02 Rigidity

Generally speaking, having multiple potential responses to a particular request is annoying. It means when a client makes a request, it has multiple message IDs it needs to wait on to find a response.

The serialization protocol doesn't support enum types, and there's no obvious way to see how to do it. However, we could simply have each different potential response be Optional. As an example, rather than having two messages each with their own id:

class RespondPuzzleState:
    puzzle_hashes: List[bytes32]
    next_height: Optional[uint32]
    next_header_hash: Optional[bytes32]
    coin_states: List[CoinState]

class RejectPuzzleState:
    header_hash: Optional[bytes32]

we could merge them:

# used in `RespondPuzzleState`
class ResponsePuzzleStateOK:
    puzzle_hashes: List[bytes32]
    next_height: Optional[uint32]
    next_header_hash: Optional[bytes32]
    coin_states: List[CoinState]

# used in `RespondPuzzleState`
class ResponsePuzzleStateFail:
    header_hash: Optional[bytes32]

# message
class RespondPuzzleState:
    ok: Optional[ResponsePuzzleStateOK]
    fail: Optional[ResponsePuzzleStateFail]

Here, exactly one of ok and fail should be not None. We can't enforce this through type-checking, but we can enforce at runtime, and simply fail on messages that don't comply like we would any other ill-formed message (for example, disconnect from or otherwise punish the remote).

We don't have any precedent for doing it this way, and this increases the size of the protocol messages and requires differentiating between the response types just the same. Message responses are identified by an id, so rather than listening for a given response type you just wait until you receive a message with the same nonce as was passed in the request.

It's pretty simple to do, here is an example I have (a bit outdated, but it shows the idea): https://github.com/Chia-Network/chia_rs/pull/369/files#diff-7f65462d1a9e187413a5013428d70e00a201bec3b2d7e0b26ce7192cbc281462R275

Rigidity avatar Feb 03 '24 04:02 Rigidity

You will usually use RequestPuzzleState and RequestCoinState as an entrypoint for subscribing, since it's more resilient to reorgs and more concise. There's a case to be made for removing the AddSubscriptions messages entirely if they don't have a use-case.

Rigidity avatar Feb 03 '24 04:02 Rigidity

We don't have any precedent for doing it this way, and this increases the size of the protocol messages and requires differentiating between the response types just the same. Message responses are identified by an id, so rather than listening for a given response type you just wait until you receive a message with the same nonce as was passed in the request.

I did notice a Optional[uint16] in the wrapper Message type. I thought I tried using that and it was None, but I will try again. It's definitely the best way I can think of to route messages, by mapping the responses to their requests with a nonce.

richardkiss avatar Feb 05 '24 20:02 richardkiss

I don't understand the benefit of this, it seems like a more complicated API to do roughly the same thing. But maybe I'm misunderstanding something? Subscribing doesn't actually return any coin states, it just registers that you're interested in a given set of puzzle hashes going forward whenever coin states are updated.

For request/responses, I see the signature conceptually as

class SomeRemoteObject:
    async def some_request(self, foo: SerializableType1, bar: SerializableType2) -> SerializableType3:
        ...

For subscriptions, I see it as

class SomeRemoteObject:
    async def subscribe_to_something(self, identifier: SerializableIdentifier) -> AsyncIterator[SubscriptionUpdate]:
        ...

Internal to this, you'd use a nonce to identify subscriptions. Remote messages containing subscription updates could use that subscription nonce to route it to the correct async iterator, and also use that nonce to inform the remote to cancel the subscription.

This makes local use really easy. You get the async iterator with the subscription api, then simply await to get items out.

[As I write this, I realize you probably actually want to return an async context manager so you can more affirmatively cancel the subscription with a remote message no matter how you exit locally.]

I realize these comments aren't really about the code. But I've been trying to write code to connect to remote chia nodes and I've found it to be much more complicated than it should be, and how certain changes could allow for better abstractions and smaller code. But maybe you don't want to tackle this now.

richardkiss avatar Feb 05 '24 20:02 richardkiss

We don't have any precedent for doing it this way, and this increases the size of the protocol messages and requires differentiating between the response types just the same. Message responses are identified by an id, so rather than listening for a given response type you just wait until you receive a message with the same nonce as was passed in the request.

I did notice a Optional[uint16] in the wrapper Message type. I thought I tried using that and it was None, but I will try again. It's definitely the best way I can think of to route messages, by mapping the responses to their requests with a nonce.

It should be the same value as the request id, if that's left as None the response will be too. If it's an update (ie CoinStateUpdate), it will also be None.

Btw I wrote the chia-client crate in this repo which helps connect to (trusted) full node peers and routes messages. It's not perfect (lacking proper error handling, disconnect handling, and validation) but it's a start.

Rigidity avatar Feb 05 '24 23:02 Rigidity

Generally speaking, having multiple potential responses to a particular request is annoying. It means when a client makes a request, it has multiple message IDs it needs to wait on to find a response.

my recollection of how our protocol driver works is that it's not waiting for message IDs, it's demultiplexing based on transaction IDs (or request IDs). So, any message can (in theory) be a response to any other. The current protocols usually have one success message and a separate failure message too.

arvidn avatar Feb 06 '24 14:02 arvidn

@richardkiss it sounds like the general features of the protocol you're describing matches what @Rigidity has documented, and is implementing.

Some major differences is that in his protocol you subscribe and request coin states in bulk, with many coin IDs and puzzle hashes. Also, subscriptions aren't given shorter names (but are just the coin ID or puzzle hash). But the updates are still delivered to the client in block-height order, making it possible for the wallet to be interrupted and resuming from the height it left off.

arvidn avatar Feb 06 '24 14:02 arvidn

We don't have any precedent for doing it this way, and this increases the size of the protocol messages and requires differentiating between the response types just the same. Message responses are identified by an id, so rather than listening for a given response type you just wait until you receive a message with the same nonce as was passed in the request.

I did notice a Optional[uint16] in the wrapper Message type. I thought I tried using that and it was None, but I will try again. It's definitely the best way I can think of to route messages, by mapping the responses to their requests with a nonce.

It should be the same value as the request id, if that's left as None the response will be too. If it's an update (ie CoinStateUpdate), it will also be None.

Your postman doesn't need to open an envelope to deliver it. Our envelope is chia.server.outbound_message.Message with the three fields, type, id and data. The type and id work together to determine where this message should be routed. (IMO, it's a bit silly to use both of these fields for this purpose, but this would be pretty tough to change at this point.)

richardkiss avatar Feb 08 '24 20:02 richardkiss

Some major differences is that in his protocol you subscribe and request coin states in bulk, with many coin IDs and puzzle hashes. Also, subscriptions aren't given shorter names (but are just the coin ID or puzzle hash). But the updates are still delivered to the client in block-height order, making it possible for the wallet to be interrupted and resuming from the height it left off

What do the updates actually look like? I don't see that in this discussion. Are we using the existing messages to do this?

richardkiss avatar Feb 08 '24 20:02 richardkiss

Some major differences is that in his protocol you subscribe and request coin states in bulk, with many coin IDs and puzzle hashes. Also, subscriptions aren't given shorter names (but are just the coin ID or puzzle hash). But the updates are still delivered to the client in block-height order, making it possible for the wallet to be interrupted and resuming from the height it left off

What do the updates actually look like? I don't see that in this discussion. Are we using the existing messages to do this?

RespondPuzzleState is used to respond to incremental requests for the initial state. CoinStateUpdate is sent every block or when a reorg occurs to give you changes for things you're subscribed to. I do not intend to drastically change how the protocol works, but rather to add missing functionality and lift some restrictions. Changing how protocol messages in general work seems pretty out of scope to me.

Rigidity avatar Feb 08 '24 23:02 Rigidity

We don't have any precedent for doing it this way, and this increases the size of the protocol messages and requires differentiating between the response types just the same. Message responses are identified by an id, so rather than listening for a given response type you just wait until you receive a message with the same nonce as was passed in the request.

I did notice a Optional[uint16] in the wrapper Message type. I thought I tried using that and it was None, but I will try again. It's definitely the best way I can think of to route messages, by mapping the responses to their requests with a nonce.

It should be the same value as the request id, if that's left as None the response will be too. If it's an update (ie CoinStateUpdate), it will also be None.

Your postman doesn't need to open an envelope to deliver it. Our envelope is chia.server.outbound_message.Message with the three fields, type, id and data. The type and id work together to determine where this message should be routed. (IMO, it's a bit silly to use both of these fields for this purpose, but this would be pretty tough to change at this point.)

It seems reasonable to me, they mean different things. The unique nonce (id) identifies which message it is, and the type identifies which kind of response you're receiving (ie a success or a failure). If you receive anything you don't expect, you ban the peer.

Rigidity avatar Feb 08 '24 23:02 Rigidity

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Feb 28 '24 18:02 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Mar 02 '24 03:03 github-actions[bot]

This is ready for initial reviews, though rate limits have not been set, so it is not ready for merge yet. That is what I will work on next.

Rigidity avatar Mar 02 '24 04:03 Rigidity

My understanding was that there was consensus (or at least some agreement) on altering the protocol to essentially merge the request for subscription with requesting coin states. and the subscription would only kick in if the complete state could be returned. Otherwise the wallet was expected to request the next range of coin states.

RequestPuzzleState syncs batch by batch until it reaches the peak and subscribes from there on, and RequestAddPuzzleSubscriptions directly adds the subscriptions without requesting initial coin data and returns the current peak as of when you subscribed.

I can get rid of RequestAddPuzzleSubscriptions, but I do like the flexibility having it offers you if you want it. And having both Add/Remove made logical sense to me. Though, I think you could technically get the same behavior by just calling RequestPuzzleState with CoinStateFilters(False, False, False, 0) to avoid getting any coins and immediately subscribe. So maybe it would just be unnecessary to have it. You could also just call RegisterForPhUpdates and ignore the response I suppose.

So I guess it's technically unnecessary?

Rigidity avatar Mar 06 '24 02:03 Rigidity

I can get rid of RequestAddPuzzleSubscriptions, but I do like the flexibility having it offers you if you want it.

Perhaps I'm missing something. Given that the wallet can't know which block will be the node's peak (by the time it receives the request) I question the usefulness of being able to just subscribe. You can't know what you're getting up front. You do see the height and block hash in the subscription response, so you do know what you ended up with.

arvidn avatar Mar 06 '24 14:03 arvidn

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Mar 14 '24 17:03 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Mar 20 '24 14:03 github-actions[bot]

I have also updated CHIP-0026 according to the new changes.

Rigidity avatar Mar 28 '24 00:03 Rigidity

I think we should either get rid of the duplicated check above if it's not important enough, or add # pragma: no cover to it. Since it's an almost impossible scenario to be in imo (and I don't know how we'd cover it with tests).

Rigidity avatar Mar 28 '24 00:03 Rigidity

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] avatar Mar 29 '24 14:03 github-actions[bot]

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions[bot] avatar Mar 29 '24 15:03 github-actions[bot]

I think it's reasonable to not cover the checks for race condition

arvidn avatar Mar 29 '24 16:03 arvidn

File Coverage Missing Lines
chia/full_node/full_node_api.py 98.2% lines 1824, 1881
Total Missing Coverage
818 lines 2 lines 99%

github-actions[bot] avatar Mar 29 '24 19:03 github-actions[bot]