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

Send inbound TransactionsById requests to the mempool storage service #1077

Closed
4 tasks
Tracked by #2309
hdevalence opened this issue Sep 20, 2020 · 6 comments · Fixed by #2725
Closed
4 tasks
Tracked by #2309

Send inbound TransactionsById requests to the mempool storage service #1077

hdevalence opened this issue Sep 20, 2020 · 6 comments · Fixed by #2725
Assignees
Labels
C-enhancement Category: This is an improvement

Comments

@hdevalence
Copy link
Contributor

hdevalence commented Sep 20, 2020

Is your feature request related to a problem? Please describe.

The zebra_network::Request::TransactionsById variant should be handled by the inbound network service.

Describe the solution you'd like

  • This page on the Bitcoin wiki https://en.bitcoin.it/wiki/Protocol_documentation#getdata suggests that transaction lookup is only allowed for mempool transactions, to prevent reliance on nodes having transaction indexes. We should check what zcashd does.

  • The inbound network service should query the mempool for those transactions and construct a response.

  • The response should consist of the found transactions, followed by a list of NotFound hashes

Additional context

See:

Blocked on actually having a mempool. Part of #889.

@hdevalence hdevalence added S-blocked Status: Blocked on other tasks C-enhancement Category: This is an improvement labels Sep 20, 2020
@teor2345 teor2345 added this to the Sync and Network milestone Sep 29, 2020
@mpguerra mpguerra removed this from the Sync and Network milestone Jan 5, 2021
@mpguerra mpguerra added this to the 2021 Sprint 17 milestone Aug 9, 2021
@mpguerra mpguerra mentioned this issue Aug 11, 2021
59 tasks
@mpguerra mpguerra changed the title Handle TransactionsByHash in the Inbound service Handle TransactionsByHash in the Inbound service Aug 11, 2021
@mpguerra mpguerra changed the title Handle TransactionsByHash in the Inbound service Handle getdata requests in TransactionsByHash in the Inbound service Aug 11, 2021
@teor2345 teor2345 changed the title Handle getdata requests in TransactionsByHash in the Inbound service Send inbound TransactionsByHash to the mempool storage service Aug 17, 2021
@teor2345 teor2345 changed the title Send inbound TransactionsByHash to the mempool storage service Send inbound TransactionsByHash requests to the mempool storage service Aug 17, 2021
@oxarbitrage oxarbitrage self-assigned this Aug 19, 2021
@oxarbitrage
Copy link
Contributor

The TransactionsByHash was renamed to TransactionsById in c608260 so i changed the title and the contents of this ticket as it confused me to create a new request but it is not needed.

@oxarbitrage oxarbitrage changed the title Send inbound TransactionsByHash requests to the mempool storage service Send inbound TransactionsById requests to the mempool storage service Aug 23, 2021
@teor2345
Copy link
Contributor

The TransactionsByHash was renamed to TransactionsById in c608260 so i changed the title and the contents of this ticket as it confused me to create a new request but it is not needed.

Thanks!

Sorry I missed this ticket, I changed most of the names last week.

@oxarbitrage
Copy link
Contributor

The reply to this message is actually 2 replies. One will have the transactions in the mempool we found but we also need to send a notfound message with a list of not found hashes.

This is what i think we can do and some questions i have:

1- Add code in https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/inbound.rs#L284 that will call https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/mempool.rs#L83 that will call https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/mempool/storage.rs#L100

2- That will get us a collection on what was found in the mempool. We can compare with the hashes in the request we have available here https://github.com/ZcashFoundation/zebra/blob/main/zebrad/src/components/inbound.rs#L284 and construct an additional response with not found hashes.

3- I am not sure on how we can reply with 2 messages in the inbound.rs as all the messages we have just reply with one. Should we wrap the 2 messages in a new response type and send that instead ?

@teor2345
Copy link
Contributor

teor2345 commented Sep 2, 2021

The reply to this message is actually 2 replies. One will have the transactions in the mempool we found but we also need to send a notfound message with a list of not found hashes.

The minimal scope for this ticket is just the transactions.
(The ticket was opened before we did the mempool scope and design.)

It looks like zcashd and Zebra don't actually need notfound messages:
https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5632
https://github.com/ZcashFoundation/zebra/blob/main/zebra-network/src/peer/connection.rs#L115

So can we open another ticket for notfound, and do it later if we have time?

@oxarbitrage
Copy link
Contributor

Ok, cool, that makes this ticket a lot easier to code. Will send a PR.

@teor2345
Copy link
Contributor

teor2345 commented Sep 2, 2021

It looks like zcashd and Zebra don't actually need notfound messages:
https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5632
https://github.com/ZcashFoundation/zebra/blob/main/zebra-network/src/peer/connection.rs#L115

So can we open another ticket for notfound, and do it later if we have time?

I opened ticket #2726 for notfound.

@mpguerra mpguerra removed the S-blocked Status: Blocked on other tasks label Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants