Skip to content

getnewblockhex: add ability to pass over transactions based on mempoo… - #339

Merged
instagibbs merged 1 commit into
elements-0.14.1from
getoldtx
Apr 27, 2018
Merged

getnewblockhex: add ability to pass over transactions based on mempoo…#339
instagibbs merged 1 commit into
elements-0.14.1from
getoldtx

Conversation

@instagibbs

Copy link
Copy Markdown
Contributor

…l age

Can be used to improve block convergence performance with compact blocks and other mempool sharing based schemes.

related to #338

Comment thread src/rpc/mining.cpp
UniValue getnewblockhex(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() != 0)
if (request.fHelp || request.params.size() > 1)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

need to list optional arg in the line 2 lines down

@jtimon jtimon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good beyond nits.

Comment thread src/miner.cpp
}

// Skip transactions that are under X seconds in mempool
if (iter->GetTime() > current_time - required_age_in_secs) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be < ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, I had it backwards first as well, which led to transactions only being accepted within first X seconds :)

Comment thread src/miner.h
* Increments nPackagesSelected / nDescendantsUpdated with corresponding
* statistics from the package selection (for logging statistics). */
void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated);
void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int required_age_in_secs=0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need the default value here too? It defaults to 0 on the rpc already.
Same for CreateNewBlock although I guess there we do need it for generate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm just passing it through, since we only want to use it with "getnewblockhex" type stuff and not generate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But only with the default value in CreateNewBlock should be enough, no?

@sipa

sipa commented Apr 26, 2018

Copy link
Copy Markdown
Contributor

utACK ed121e8. Is the Travis failure expected?

@instagibbs

Copy link
Copy Markdown
Contributor Author

@sipa we're not sure... it's 1 of 2 travis runs that is failing the gpg signature checks. We've had this before but can't recall if we just ignored it or fixed it somehow...

@instagibbs
instagibbs merged commit ed121e8 into elements-0.14.1 Apr 27, 2018
instagibbs added a commit that referenced this pull request Apr 27, 2018
…ed on mempoo…

ed121e8 getnewblockhex: add ability to pass over transactions based on mempool age (Gregory Sanders)
@instagibbs

Copy link
Copy Markdown
Contributor Author

merging, hoping the issue is something that will go away...

@instagibbs

Copy link
Copy Markdown
Contributor Author

yep tip seems fine

@jtimon
jtimon deleted the getoldtx branch May 4, 2018 01:31
instagibbs added a commit that referenced this pull request Oct 29, 2018
c3fe935 test compact block rpcs (Gregory Sanders)
c3cbe6e compact block RPC calls (Gregory Sanders)

Pull request description:

  Combined with #339 should be pretty powerful for speed of block proposal transmission.

Tree-SHA512: 305256b844a28542bb720ab86a36ec91982dd41c4d8e455e34ad3db12daa6e8b25ccfe2728fa514812655c2121c8d1d36e71f2af83f86b2de6c5d0c72541d79e
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