Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Pending transactions subscription #7906

Merged
merged 6 commits into from
Feb 16, 2018
Merged

Pending transactions subscription #7906

merged 6 commits into from
Feb 16, 2018

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Feb 15, 2018

Resolves #6426

@rphmeier what's your thoughts on light client? Should we notify about every transaction in light queue?

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Feb 15, 2018
@rphmeier
Copy link
Contributor

I think that makes sense. I'm not sure how useful that information is because only local transactions are queued on the light client.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure if you're going to change the light client behavior.

From geth documentation (https://github.com/ethereum/go-ethereum/wiki/RPC-PUB-SUB#newpendingtransactions):

When a transaction that was previously part of the canonical chain isn't part of the new canonical chain after a reogranization its again emitted.

Do/should we have the same behavior?

}

result
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe wrap the result in Ok here, this way we don't need the if above.

@5chdn 5chdn added this to the 1.10 milestone Feb 16, 2018
@tomusdrw
Copy link
Collaborator Author

Do/should we have the same behavior?

Yes, it notifies about all transactions that are inserted to the queue (and transactions from retracted blocks are re-inserted).

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 16, 2018
@5chdn 5chdn merged commit 236a4aa into master Feb 16, 2018
@5chdn 5chdn deleted the td-pending-tx branch February 16, 2018 15:51
@tzapu
Copy link
Contributor

tzapu commented Feb 17, 2018

hi guys, wonderful to have this.

just to clarify:

  • in light mode only local transactions are in the pool and sent through the subscription
  • in full mode everything added to the pending pool is sent through

did i get it right?
Cheers

geth docs seem to suggest that only local pendings would go out through this subscription
this would make it work wildly different than the pending transactions filter though...

@tomusdrw
Copy link
Collaborator Author

@tzapu Correct. Yes, I saw a suggestion in geth docs, although checked the code and they seem to push every transaction to the subscription (which IMHO makes sense, TBH we should also inform about transactions that are dropped/replaced in the pool - I will prepare a parity-specific method for that in the future). Let me know if the behaviour is undesirable, we can change it.

@tzapu
Copy link
Contributor

tzapu commented Feb 18, 2018

well, ideally, my use case would benefit from feature parity between modes, so light mode also gets the entirety of pending transactions, but i get that that might be undesirable, unwarranted or maybe not even possible

other than that, this is perfect. also, the dropped/replaced subscription or call would be absolutely wonderful

thanks again for all your hard work

@rphmeier
Copy link
Contributor

@tzapu the behavior on light clients is because light clients don't even accept or re-propagate transactions from the rest of the network. even the basic validity checks of "does this sender have enough of a balance to pay the fee" and "is this transaction valid in the future or valid now?" are quite expensive for light clients to handle.

@tzapu
Copy link
Contributor

tzapu commented Feb 18, 2018

thanks for the clarification, i imagined there must be some kind of limitations
love the progress on everything 👍 🎉 🎈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants