Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Add GetRawBlocks and GetCFilters gRPC methods #1705

Merged
merged 3 commits into from
Jun 12, 2020

Conversation

matheusd
Copy link
Member

This adds the GetCFilters method to the WalletService gRPC service. This
allows clients to fetch the wallet-stored and header-validated version 2
committed filters so that non-wallet-related transactions can be
searched for.

It also adds a new service (NetworkService) with a GetRawBlock call. The
service is intended to be used for calls which fetch or push data that
might not be backed by the wallet into the P2P network.

This particular method is useful for external callers to fetch a full
block after they have determined it (probably) contains transactions
they are interested in by checking the respective cfilter.

A previous commit also fixes an issue in regenerating the api bindings from the api.proto file.

@matheusd matheusd mentioned this pull request Mar 30, 2020
9 tasks
rpc/regen.sh Outdated Show resolved Hide resolved
internal/rpc/rpcserver/server.go Outdated Show resolved Hide resolved
}
}

blocks, err := n.Blocks(ctx, []*chainhash.Hash{bh})
Copy link
Member

@jrick jrick Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may cause some problems in SPV mode if it is called concurrently with a request for the same block from the SPV syncer. These concurrent requests are disallowed by the p2p package (a restriction that allows for the more synchronous API).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. I could add a mutex to RemotePeer to protect concurrent invocations to Blocks()/Block(). Though it's going to be tricky to handle the cancelling of the ctx (the lock might be acquired only after the peer has already beed disconnected due to errors and the caller's ctx may have already been canceled so I'd need to handle all that)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added mutex in 57dd618

internal/rpc/rpcserver/server.go Outdated Show resolved Hide resolved
@matheusd
Copy link
Member Author

matheusd commented Apr 1, 2020

I have another issue to handle as well (either on this or on a separate PR):

The wallet won't relay non-wallet-relevant txs due to not storing them. Any suggestions on how to handle this?

My initial ideas as alternatives:

  • Add a sort of mempool to Wallet{} that would be checked whenever the spv syncer's receiveGetData is signalled to relay a new tx. Handling removing the txs on a given schedule would be tricky.

  • Add callbacks on the wallet to create a client backed mempool. When querying for a tx, query via these callbacks. Might also be tricky, have possible recursion issues.

  • Add a new method to Network implementations: ForcePublishTx. On RPC this maps to the regular PublishTransaction but on SPV this would map to something that publishes and waits for a corresponding GetData call to happen before returning.

@jrick
Copy link
Member

jrick commented Apr 1, 2020

Another possibility would be to send the tx out immediately instead of sending the inv first and waiting for a getdata. I don't like allowing this though...

If we add a temporary "mempool" for publishing recently inv'd transactions that aren't saved by the wallet, removing them after some duration, I think it's more appropriate to add that functionality to the SPV syncer instead of the wallet, since it really is the brains behind how the entire application interacts with the network and what messages are sent (vs p2p which is dumber and is mostly for directly sending and receiving those messages, and vs wallet which we don't want to taint with unrelated transactions).

@matheusd
Copy link
Member Author

matheusd commented Apr 2, 2020

I think it's more appropriate to add that functionality to the SPV syncer

I considered that. The only tricky bit is determining when to store in the syncer's mempool or not. I'd have to either pass another argument to PublishTransactions or determine (during PublishTransactions) whether that tx is in the wallet or not to store it (but that would involve doing another db op which increases latency when sending out votes).

I'll poc using an additional argument and see if that's acceptable.

@matheusd
Copy link
Member Author

matheusd commented Apr 3, 2020

Added the mempool to SPV syncer in 9527f8c

I guessed an eviction timeout of 60 minutes, can adjust as needed.

I see there's a conflict with current master, I'll rebase and fix that after you've had a chance to take a look at the current solution.

@jrick
Copy link
Member

jrick commented Apr 8, 2020

I think we should be able to avoid the signature change on PublishTransactions. The SPV syncer can call the wallet to determine if a transaction is relevant or not to determine whether to add it to the wallet, or manage it itself in memory.

Currently the wallet has an unexported isRelevantTx method for this, but we could provide an exported version.

@matheusd
Copy link
Member Author

Changed last commit from changing the PublishTransactions signature to using DetermineRelevantTxs (so that only a single dbtx is done for the whole set of txs).

This adds the GetCFilters method to the WalletService gRPC service. This
allows clients to fetch the wallet-stored and header-validated version 2
committed filters so that non-wallet-related transactions can be
searched for.

It also adds a new service (NetworkService) with a GetRawBlock call. The
service is intended to be used for calls which fetch or push data that
might not be backed by the wallet into the P2P network.

This particular method is useful for external callers to fetch a full
block after they have determined it (probably) contains transactions
they are interested in by checking the respective cfilter.
This fixes a case where a transaction not found during
GetTransactionsByHashes would not be correctly added to the notFound
return slice.
This adds a mempool on the SPV Syncer so that it can actually relay
transactions that are not relevant to the wallet.

Transactions that are relevant (i.e. stored in the wallet's DB) don't
need to be stored in the mempool since they are already accessible when
a GetData request comes in from a peer.
Copy link
Member

@jrick jrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reads well and I don't anticipate any issues anymore after the p2p changes to allow concurrent requests of the same data.

@jrick jrick merged commit 25f714d into decred:master Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants