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

add start_newPendingTransactions_loop #2690

Conversation

waelsy123
Copy link

@waelsy123 waelsy123 commented May 28, 2023

This change is Reviewable

Support for new pending transactions pubsub #2672

[WIP]
do we have a notification Channel to subscribe for when new pending transaction? so far I'm using the new_block_hashes channel

@Conflux-Dev
Copy link
Contributor

Can one of the admins verify this patch?

@peilun-conflux
Copy link
Contributor

ok to test

@peilun-conflux
Copy link
Contributor

There is no such a channel, and that's probably why newPendingTransactions was not supported in the first place.

One possible choice is to create a new channel for this and push successfully inserted transaction hashes in TransactionPool::insert_new_transactions(). And we also need to filter out transaction spaces for RPCs. I'm not quite sure if this exactly meet the definition of new pending transactions pubsub though.

@waelsy123
Copy link
Author

@peilun-conflux I just created a new channel new_pending_transactions and notified subscribers with new hashes inserted in the queue. what do you think about that?

Copy link
Contributor

@peilun-conflux peilun-conflux left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @waelsy123)


client/src/rpc/impls/eth_pubsub.rs line 117 at r2 (raw file):

        let fut = async move {
            while let Some(new_tx) = receiver.recv().await {
                debug!("new_pending_transactions_loop: {:?}", new_tx);

Since transactions can be received very fast, it's better to remove this or change it to trace!.


client/src/rpc/impls/eth_pubsub.rs line 292 at r2 (raw file):

    // notify each subscriber about new pending transaction `hash` concurrently
    fn notify_new_pending_transaction(&self, hash: H256) {

Same about the log level here.

@peilun-conflux
Copy link
Contributor

@peilun-conflux I just created a new channel new_pending_transactions and notified subscribers with new hashes inserted in the queue. what do you think about that?

Looks good to me.

BTW, you need to run ./cargo_fmt.sh --install to format the code so it can pass the CI check. After installing the (very old) hardcoded cargo version for the first time, running ./cargo_fmt.sh should be enough later.

@waelsy123
Copy link
Author

waelsy123 commented Jun 10, 2023

@peilun-conflux thank you for the feedback!
I just used trace for better logs level,

regarding the formating, seems like rustup doesn't support my m1 mac arch, I'm getting this error:


%  ./cargo_fmt.sh --install                         
info: syncing channel updates for 'nightly-2019-07-03-aarch64-apple-darwin'
info: latest update on 2019-07-03, rust version 1.37.0-nightly (0beb2ba16 2019-07-02)
error: target 'aarch64-apple-darwin' not found in channel.  Perhaps check https://doc.rust-lang.org/nightly/rustc/platform-support.html for available targets

it's worth to mention I'm using rustup 1.26.0 (5af9b9484 2023-04-05)

I'm curious why do we use hardcoded nightly-2019-07-03 ?

@CRossel87a
Copy link

There are already flashbots working in the mempool of Conflux. Is a beta version out?

@Pana
Copy link
Member

Pana commented Mar 15, 2024

@waelsy123 can you fix the conflict issue?

@Pana Pana closed this Aug 26, 2024
@waelsy123
Copy link
Author

@Pana sure will fix it

@waelsy123
Copy link
Author

@Pana is that still valid PR?

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.

5 participants