Skip to content

Remove txmempool implicit-integer-sign-change sanitizer suppressions #23957

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
Closed
wants to merge 1 commit into from
Closed

Remove txmempool implicit-integer-sign-change sanitizer suppressions #23957

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 3, 2022

A file-wide suppression is problematic because it will wave through future violations, potentially bugs.

Fix that by using per-statement casts.

This refactor doesn't change behavior because the now explicit casts were previously done implicitly.

Similar to commit 8b5a4de

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 3, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23629 (refactor tests to fix ubsan suppressions by MarcoFalke)
  • #21464 (Mempool Update Cut-Through Optimization by JeremyRubin)
  • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto
Copy link
Member

hebasto commented Jan 3, 2022

Could we just avoid conversion from int64_t to size_t in

bitcoin/src/txmempool.cpp

Lines 114 to 117 in 75a227e

size_t CTxMemPoolEntry::GetTxSize() const
{
return GetVirtualTransactionSize(nTxWeight, sigOpCost);
}
and return int64_t?

@maflcko
Copy link
Member Author

maflcko commented Jan 3, 2022

Yes, potentially. Though, the goal of this pull is to keep all integer widths and types the same, only make implicit conversions explicit for the reasons mentioned in OP. Also, changing the return type of GetTxSize won't fix the implicit conversion for modifySize, which is already int64_t, so I think the changes here are needed either way.

@maflcko
Copy link
Member Author

maflcko commented Jan 3, 2022

I've checked that the binary doesn't change with clang -O2 on my system. (It does change with gcc, seemingly because functions are for some reason re-ordered in the object file).

@hebasto
Copy link
Member

hebasto commented Jan 3, 2022

Yes, potentially. Though, the goal of this pull is to keep all integer widths and types the same, only make implicit conversions explicit for the reasons mentioned in OP.

A conversion sequence int64_t --> size_t --> int64_t looks unneeded, doesn't it? Maybe it deserves its own PR?

Also, changing the return type of GetTxSize won't fix the implicit conversion for modifySize, which is already int64_t, so I think the changes here are needed either way.

Sure. ACK on uint64_t(modifySize) and uint64_t(modifyCount).

@maflcko
Copy link
Member Author

maflcko commented Jan 3, 2022

Maybe it deserves its own PR?

Yes, this should be done in another PR. I briefly looked into this, but this touches policy and miner as well. (It is a bit of a can of worms like #23633)

hebasto
hebasto approved these changes Jan 3, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa4314f, I have reviewed the code and it looks OK, I agree it can be merged.

@DrahtBot DrahtBot mentioned this pull request Jan 3, 2022
Merged
@hebasto hebasto mentioned this pull request Jan 3, 2022
Merged
@hebasto
Copy link
Member

hebasto commented Jan 3, 2022

Maybe it deserves its own PR?

Yes, this should be done in another PR.

Done in #23962.

This was referenced Jan 4, 2022
Merged
Merged
shaavan
shaavan approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK fa4314f

  • I agree with the motivation of this PR.

A file-wide suppression is problematic because it will wave through future violations, potentially bugs.

  • Checked that each variable is properly typecasted explicitly.
  • Since the line implicit-integer-sign-change:txmempool.cpp has been removed from santizer_supression/urban list, and the CI passed successfully, that means as of now, there are no more variables in txmempool.cpp that requires explicit type conversion.

@maflcko
Copy link
Member Author

maflcko commented Jan 4, 2022

Done in #23962.

Thanks, closing this for now to avoid merge conflicts.

@maflcko maflcko closed this Jan 4, 2022
@maflcko maflcko deleted the 2201-remTxPool branch January 4, 2022 09:33
@glozow glozow mentioned this pull request Jun 22, 2022
Merged
2 tasks
@bitcoin bitcoin locked and limited conversation to collaborators Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@shaavan shaavan shaavan approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @DrahtBot @hebasto @shaavan