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

fix: remove unbounded vec allocations from base node grpc/p2p messaging #3467

Merged
merged 5 commits into from Oct 19, 2021

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Oct 18, 2021

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 #3310

How Has This Been Tested?

Existing GRPC tests
Manually tested block explorer api connected to local base node
Additional unit tests

@sdbondi sdbondi force-pushed the core-fetch-headers branch 2 times, most recently from badb254 to 03d2b45 Compare October 18, 2021 06:16
- 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
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Nice, but let's rather change the start and end to use a range and avoid inconsistencies

stringhandler
stringhandler previously approved these changes Oct 19, 2021
* development:
  feat: tx weight takes tariscript and output features into account [igor] (tari-project#3411)
@aviator-app aviator-app bot removed the mq-failed label Oct 19, 2021
@aviator-app aviator-app bot merged commit 5d7fb20 into tari-project:development Oct 19, 2021
@sdbondi sdbondi deleted the core-fetch-headers branch October 20, 2021 04:47
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 25, 2021
* development: (31 commits)
  feat!: revalidate all outputs (tari-project#3471)
  fix: check SAF message inflight and check stored_at is in past (tari-project#3444)
  feat!: apps should not depend on other app configs (tari-project#3469)
  fix: fix recovery test reporting message (tari-project#3479)
  chore: improve cucumber tests to wait for broadcast (tari-project#3461)
  test: use TCP node for daily sync test (tari-project#3464)
  fix: remove unbounded vec allocations from base node grpc/p2p messaging (tari-project#3467)
  fix: upgrade rustyline dependencies (tari-project#3476)
  fix(dht): discard encrypted message with no destination (tari-project#3472)
  fix: remove consensus breaking change in transaction input (tari-project#3474)
  feat: tx weight takes tariscript and output features into account [igor] (tari-project#3411)
  fix: validate dht header before dedup cache (tari-project#3468)
  fix: sha256sum isn't available on all *nix platforms (tari-project#3466)
  fix: typo in console wallet (tari-project#3465)
  fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462)
  fix: remove is_synced check for transaction validation (tari-project#3459)
  feat: improve logging for tari_mining_node (tari-project#3449)
  fix: remove unnecessary wallet dependency (tari-project#3438)
  test: simplify cucumber tests (tari-project#3457)
  ci: create script to update DNS records from hashes.txt (tari-project#3458)
  ...
@sdbondi sdbondi restored the core-fetch-headers branch February 3, 2022 05:29
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.

RPC command fetch_headers generates in memory vector
2 participants