Skip to content

[pr] Fix solana-runtime#2

Merged
DudessaPr merged 6 commits intofeat/risc0from
feat/runtime
Jul 10, 2024
Merged

[pr] Fix solana-runtime#2
DudessaPr merged 6 commits intofeat/risc0from
feat/runtime

Conversation

@DudessaPr
Copy link
Copy Markdown

Problem

Summary of Changes

Alleviating complications with importing solana-runtime into svm-rollup

Fixes #

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Still weird but if the compiler wants it, so ok... 🤷😄

Copy link
Copy Markdown

@neutrinoks neutrinoks left a comment

Choose a reason for hiding this comment

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

Looks very good and clean to me 🚀

Copy link
Copy Markdown

@Yiwen-Gao Yiwen-Gao left a comment

Choose a reason for hiding this comment

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

I left a couple of questions, but the PR looks good to me overall. Thanks!

pubkey::Pubkey,
},
std::{
io::{Read, Write},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: can we comment this out instead of deleting it so that we know what our changes are when looking back? 🙏

foundation_rate,
prev_epoch_duration_in_years,
capitalization,
validator_rate: _,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why is this needed? For types?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That is ignored variables. We can put :_ to ignore them as their unused in the function. Simply it is to avoid complains from cargo clippy

Copy link
Copy Markdown
Author

@DudessaPr DudessaPr Jul 10, 2024

Choose a reason for hiding this comment

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

These variables are not used. They use interesting property of Rust, they return a struct and instead of creating an object corresponding to that struct they declare its fields. I used ":_" in order to avoid cargo clippy complains about unused variables.

for instance

warning: unused variable: `validator_rate`
   --> runtime/src/bank/partitioned_epoch_rewards/calculation.rs:109:13
    |
109 |             validator_rate,
    |             ^^^^^^^^^^^^^^ help: try ignoring the field: `validator_rate: _`
    |
    = note: `#[warn(unused_variables)]` on by default
  

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Got it, thanks for the context.

Comment thread runtime/src/bank.rs
prev_epoch_duration_in_years,
validator_rate,
foundation_rate,
prev_epoch_duration_in_years: _,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same question here.

Comment thread runtime/src/bank.rs
} else {
MAX_TRANSACTION_FORWARDING_DELAY_GPU
};
// let api = perf_libs::api();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is perf_libs from the solana-performance crate?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, you are right

@Yiwen-Gao
Copy link
Copy Markdown

@DudessaPr can you squash d1ec319 (apply risc0 changes) and f48695c (address warnings) into a single commit to clean up the history on this branch?

Then when you merge this into feat/risc0, can you select "Rebase and merge" instead of "Squash and merge" so that we maintain each commit as a separate changeset? Thanks!

DudessaPr and others added 6 commits July 10, 2024 14:19
apply risc0 changes

lint

resolve some warnings

return

address warnings
* remove ztsd

* remove lz4

* remove gz and fix others

* remove filetime/tar

* remove perf from runtime

* fix accountsdb const

* comment out solana perf

* remove symlink

* update cargo lock

* remove flate2 again

* retrun bz instead of hz

* try brackets

* revert

return removed and comment
@DudessaPr DudessaPr merged commit fdbfe54 into feat/risc0 Jul 10, 2024
petarvujovic98 pushed a commit that referenced this pull request Oct 24, 2025
* Apply the retry code to the async pubsub client

Create a test server

```ts
import http from "http";

import { WebSocketServer } from "ws";

let attemptCount = 0;

const server = http.createServer();
const wss = new WebSocketServer({ noServer: true });

wss.on("connection", (ws) => {
  ws.send("Connection accepted.");
  ws.on("message", (msg) => console.log(`Received: ${msg}`));
});

server.on("upgrade", (req, socket, head) => {
  attemptCount += 1;

  if (attemptCount <= 4) {
    socket.write("HTTP/1.1 429 Too Many Requests\r\n\r\n");
    socket.destroy();
    console.log(`Rejected connection #${attemptCount} (429)`);
    return;
  }

  wss.handleUpgrade(req, socket, head, (ws) => {
    wss.emit("connection", ws, req);
    console.log("Connection accepted on attempt", attemptCount);
  });
});

server.listen(8080, () => {
  console.log("Server listening on port 8080");
});
```

Run `test_slot_subscription_async`:

```
Rejected connection #1 (429)
Rejected connection #2 (429)
Rejected connection #3 (429)
Rejected connection #4 (429)
Connection accepted on attempt 5
Received: {"id":1,"jsonrpc":"2.0","method":"slotSubscribe","params":[]}
```

* `s/async_with_retry/with_retry/`
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.

3 participants