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

Fixed ban bug that doesn't print numTxns #1900

Merged
merged 1 commit into from
Oct 10, 2022

Conversation

l0k18
Copy link
Contributor

@l0k18 l0k18 commented Oct 10, 2022

This is a typo fix. There is an underlying ban bug related to this that causes two btcd nodes to ban each other due to "transaction not found" errors.

(cherry picked from commit 11c8d11)
@jcvernaleo
Copy link
Member

Good catch. Just waiting for tests to run then I'll review.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3219535761

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-11.1%) to 55.132%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server.go 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 2 86.07%
peer/peer.go 4 73.2%
Totals Coverage Status
Change from base Build 3216328982: -11.1%
Covered Lines: 26519
Relevant Lines: 48101

💛 - Coveralls

@jcvernaleo
Copy link
Member

@l0k18 looks like a test fails. Could you check that out?

@l0k18
Copy link
Contributor Author

l0k18 commented Oct 10, 2022

https://github.com/Indra-Labs/btcd/commits/tx-not-found-error-bug

Literally only changes one variable name specified in the error print. This test failure is not my work.

@l0k18
Copy link
Contributor Author

l0k18 commented Oct 10, 2022

image

https://github.com/btcsuite/btcd/actions/runs/3219535761/jobs/5265307632

I don't see how this change would have affected this test but if so, the test is wrong.

The failing test passes on my system.

https://github.com/btcsuite/btcd/blob/master/addrmgr/addrmanager_internal_test.go#L171 is the code location that produces this test fail causing error.

Not sure how this slipped through previously but that looks like a race condition to me, somewhere else this a.addrindex is being altered without being locked.

I have not looked deeply at the relevant code but in my experience, very often it's only one or two values that need to be locked and atomics are more bug resistant than mutexes.

@jcvernaleo
Copy link
Member

Well, looks like it was one of the random failures we see with GH Actions occasionally (still better than travis though). Sorry for the extra trouble there.

Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

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

OK

@jcvernaleo jcvernaleo merged commit e563459 into btcsuite:master Oct 10, 2022
@l0k18
Copy link
Contributor Author

l0k18 commented Oct 10, 2022

I am pretty sure there is a race in this bug. I'd put a note on it to check. If that variable is accessed anywhere else without the mutex locked this could cause it.

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