Skip to content

test: add functional test for txrelay during and after IBD #19423

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
Jul 17, 2020
Merged

test: add functional test for txrelay during and after IBD #19423

maflcko merged 1 commit into from
Jul 17, 2020

Conversation

@glozow
Copy link
Member

@glozow glozow commented Jul 1, 2020 •
edited
Loading

This is a followup to #19204 which uses minfeefilter=MAX_MONEY to effectively shut off txrelay, thereby reducing inv traffic, when nodes are in IBD. It was missing a functional test.

@fanquake fanquake added the Tests label Jul 1, 2020
@glozow
Copy link
Member Author

glozow commented Jul 1, 2020

Btw thanks @jonatack for input on the test 😄 added you as coauthor. I added a loop for peerinfo so this test should work with any number of nodes.

@glozow glozow force-pushed the ibd-txrelay-test branch from 502950d to c33c390 Compare July 1, 2020 05:46
@glozow
Copy link
Member Author

glozow commented Jul 1, 2020

Pushing to rerun travis because a job timed out :(

jonatack
jonatack reviewed Jul 1, 2020
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK 😄

A few suggestions below.

@jonatack
Copy link
Member

jonatack commented Jul 1, 2020 •
edited
Loading

Pushing to rerun travis because a job timed out :(

You can also ask for a travis re-kick on irc and someone will restart just the recalcitrant job. In this case, I'll keep a sharp weather eye and re-kick any jobs needed here.

@glozow glozow force-pushed the ibd-txrelay-test branch from c33c390 to 236cd1c Compare July 1, 2020 06:02
@jonatack
Copy link
Member

jonatack commented Jul 1, 2020

LGTM

naumenkogs
naumenkogs reviewed Jul 1, 2020
View reviewed changes
maflcko
maflcko reviewed Jul 1, 2020
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Very nice. Concept ACK

@glozow glozow force-pushed the ibd-txrelay-test branch from 236cd1c to fc1a714 Compare July 2, 2020 18:24
@maflcko
Copy link
Member

maflcko commented Jul 2, 2020

ACK fc1a714

@glozow
Copy link
Member Author

glozow commented Jul 2, 2020 •
edited
Loading

Status: I think fc1a714 (with just the minfeefilter) is sufficient for testing #19204.

I figured out a way to test transaction relay too in 7bc653e, but it's not perfect - there isn't an obvious way to see what tx invs a node has received since bitcoind doesn't provide that information through rpc.

jnewbery
jnewbery reviewed Jul 3, 2020
View reviewed changes
@glozow glozow force-pushed the ibd-txrelay-test branch 2 times, most recently from 9bdbd30 to 6cd6fc8 Compare July 3, 2020 20:26
@glozow
Copy link
Member Author

glozow commented Jul 3, 2020

Soooo I just had an excellent adventure through the mock time stuff... I can't get -mocktime to work (see this test which passes for setmocktime but not -mocktime) so I implemented this by fastforwarding one node by 2 days so it thinks it's still in IBD even when receiving blocks (all > 1 day old). Then we don't need to restart the node.

@promag
Copy link
Contributor

promag commented Jul 7, 2020 •
edited
Loading

Concept ACK, just read the code and looks good to me.

@practicalswift
Copy link
Contributor

practicalswift commented Jul 10, 2020

Concept ACK

Thanks for improving our testing!

@maflcko
Copy link
Member

maflcko commented Jul 11, 2020

The test fails on the second commit. I am happy to merge the first commit, though, and we can leave the second commit for a later pull?

@glozow
Copy link
Member Author

glozow commented Jul 11, 2020

@MarcoFalke thanks! Would you happen to have a log for the test fail? (Also it seems like a travis job timed out but otherwise green). I can definitely separate the second commit, but currently it's working for me so I'm not sure how to fix it 😕

@jonatack
Copy link
Member

jonatack commented Jul 11, 2020

@gzhao408 here's the link (you can click through to it on the ❌ ) https://travis-ci.org/github/bitcoin/bitcoin/jobs/704781135. I restarted the job a few days ago to see if it would re-fail.

@jonatack
Copy link
Member

jonatack commented Jul 11, 2020

(Do you have travis linked to your personal fork of the bitcoin repo? You can restart failing jobs there.)

@glozow glozow force-pushed the ibd-txrelay-test branch from 6cd6fc8 to 52e6a4e Compare July 11, 2020 18:49
@glozow glozow force-pushed the ibd-txrelay-test branch from 52e6a4e to cb31ee0 Compare July 15, 2020 23:42
@glozow
Copy link
Member Author

glozow commented Jul 16, 2020

Sorry for the delay 😓 ready for review again.

Pushed it back to the first commit and fixed the error - this tests the fee filter during and after IBD, no txrelay. I'll work on txrelay and will make it a followup.

@jnewbery
Copy link
Contributor

jnewbery commented Jul 16, 2020

utACK cb31ee0

@glozow glozow requested a review from maflcko July 17, 2020 02:52
@maflcko maflcko merged commit 19aaf79 into bitcoin:master Jul 17, 2020
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 2, 2021
@glozow @PiRK
[test] feefilter during and after IBD
ca15bc5 
Summary:
> This is a followup to #19204 which uses minfeefilter=MAX_MONEY to effectively shut off txrelay, thereby reducing inv traffic, when nodes are in IBD. It was missing a functional test.

Co-authored-by: Jon Atack <[email protected]>

This is a backport of [[bitcoin/bitcoin#19423 | core#19423]]

Test Plan: `ninja check-functional`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10014
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@maflcko maflcko Awaiting requested review from maflcko

3 more reviewers

@jnewbery jnewbery jnewbery left review comments

@jonatack jonatack jonatack left review comments

@naumenkogs naumenkogs naumenkogs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@glozow @jonatack @maflcko @promag @practicalswift @jnewbery @naumenkogs @fanquake