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.

Sign up for GitHub

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

Jump to bottom

RPC command fetch_headers generates in memory vector #3310

Closed
SWvheerden opened this issue Sep 7, 2021 · 0 comments · Fixed by #3467
Closed

RPC command fetch_headers generates in memory vector #3310

SWvheerden opened this issue Sep 7, 2021 · 0 comments · Fixed by #3467

Comments

@SWvheerden
Copy link
Collaborator

The RPC command fetch_headers on the server-side will generate a vector of block headers in memory before trying to send it.
Assuming the current tip height of around 20900, this will be of size 4.953 MB. Assuming only Sha3 blocks which are smaller than Monero blocks. This will grow in size as the blockchain grows, assuming 4mb max packets, to roughly block height 500 000. The server then needs a vector of size 119mb to keep all the headers in.
@SWvheerden SWvheerden changed the title RPC command fetch_headers RPC command fetch_headers generates in memory vector Sep 7, 2021
@SWvheerden SWvheerden mentioned this issue Sep 9, 2021
Closed
@sdbondi sdbondi mentioned this issue Oct 18, 2021
Merged
Cifko pushed a commit to Cifko/tari that referenced this issue Oct 20, 2021
@sdbondi
fix: remove unbounded vec allocations from base node grpc/p2p messagi…
5d7fb20 
…ng (tari-project#3467)

Description
---
- replaces unbounded height lists with bound height ranges in internal base node service
  interfaces and p2p messaging
- removes unused and deprecated base node p2p messaging requests/responses
- load chain headers when requesting headers, this simplifies and
  reduces the database load when calling grpc get_blocks
- use iterator with almost no footprint for paging in grpc calls
- implement `DoubleSidedIterator` for `NonOverlappingIntegerPairIter`
- add overflow protection to `NonOverlappingIntegerPairIter` and
  additional tests

This PR does not contain any breaking changes, as no nodes using the base node p2p messaging interface except for block propagation (which is unchanged). GRPC protobuf contract is preserved.

**New dependency**
`either = "1.6.1"` added to `tari_base_node` crate and used for its `Either` iterator impl

Motivation and Context
---
- removing unused "outbound comms interface" request/responses reduces attack surface (TODO: remove this interface entirely - specifically the coupling for internal service requests and external p2p comms is troublesome, but mostly removed in the PR, but removing it entirely will make making changes to the base node service much easier)
- remote peers could request any number of heights allowing an external party to allocate memory without bounds 

Closes tari-project#3310 

How Has This Been Tested?
---
Existing GRPC tests
Manually tested block explorer api connected to local base node
Additional unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: remove unbounded vec allocations from base node grpc/p2p messaging sdbondi/tari
1 participant
@SWvheerden