Skip to content

refactor: remove unused methods in classes CDBIterator,CDBWrapper,CCoinsViewDBCursor #25438

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 2 commits into from
Jun 24, 2022
Merged

refactor: remove unused methods in classes CDBIterator,CDBWrapper,CCoinsViewDBCursor #25438

maflcko merged 2 commits into from
Jun 24, 2022

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Jun 21, 2022 •
edited
Loading

The GetValueSize methods haven't been used since the chainstate db cache has been switched from per-tx to per-txout model years ago (PR #10195, commit d342424). The CompactRange is unused since the txindex migration code was removed (PR #22626, commit fa20f81).

@theStack
refactor: remove unused methods `{CDBIterator,CCoinsViewDBCursor}::Ge…
fb38c6e 
…tValueSize()`

These methods haven't been used since the chainstate db cache has been
switched from per-tx to per-txout model years ago (PR bitcoin#10195, commit
d342424).
w0xlt
w0xlt approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Code Review ACK fb38c6e

@laanwj
Copy link
Member

laanwj commented Jun 22, 2022

Code review ACK fb38c6e

furszy
furszy approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK fb38c6e

@furszy
Copy link
Member

furszy commented Jun 22, 2022

Btw, checking at this dbwrapper.h file, isn't CompactRange unused as well?

@laanwj
Copy link
Member

laanwj commented Jun 23, 2022 •
edited
Loading

Btw, checking at this dbwrapper.h file, isn't CompactRange unused as well?

Yes. It seems like it!
A direct call to leveldb's CompactRange is used in CDBWrapper::CDBWrapper in -forcecompactdb handling. But our own function is never used.
The last use went away in #22626.

@theStack @furszy
refactor: remove unused method CDBWrapper::CompactRange
e4b4db5 
This method hasn't been used since the txindex migration code has been
removed (PR bitcoin#22626, commit fa20f81).

Co-authored-by: furszy <[email protected]>
@theStack theStack changed the title refactor: remove unused methods {CDBIterator,CCoinsViewDBCursor}::GetValueSize() refactor: remove unused methods in classes CDBIterator,CCoinsViewDBCursor Jun 23, 2022
@theStack
Copy link
Contributor Author

theStack commented Jun 23, 2022

@furszy @laanwj: Thanks a lot, good catch! Added another commit and updated the PR title / description accordingly.

fanquake
fanquake approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK e4b4db5

@theStack theStack changed the title refactor: remove unused methods in classes CDBIterator,CCoinsViewDBCursor refactor: remove unused methods in classes CDBIterator,CDBWrapper,CCoinsViewDBCursor Jun 23, 2022
furszy
furszy approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

re-ACK e4b4db5

@maflcko
Copy link
Member

maflcko commented Jun 23, 2022

Clear may be unused as well? https://marcofalke.github.io/btc_cov/total.coverage/src/dbwrapper.h.gcov.html#66

@theStack
Copy link
Contributor Author

theStack commented Jun 23, 2022

Clear may be unused as well? https://marcofalke.github.io/btc_cov/total.coverage/src/dbwrapper.h.gcov.html#66

Seems like it is used in CCoinsViewDB::BatchWrite (

bitcoin/src/txdb.cpp

Line 154 in 01e9e2d

batch.Clear();
):

txdb.cpp:154:19: error: no member named 'Clear' in 'CDBBatch'
            batch.Clear();
            ~~~~~ ^
1 error generated.
gmake[2]: *** [Makefile:10066: libbitcoin_node_a-txdb.o] Error 1

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 23, 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:

  • #25296 (Add DataStream without ser-type and ser-version and use it where possible by MarcoFalke)
  • #24232 (assumeutxo: add init and completion logic by jamesob)

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.

@DrahtBot DrahtBot mentioned this pull request Jun 23, 2022
Closed
laanwj
laanwj approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member

@laanwj laanwj left a comment

Choose a reason for hiding this comment

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

Code review ACK e4b4db5

@DrahtBot DrahtBot mentioned this pull request Jun 23, 2022
Closed
1 task
@maflcko maflcko merged commit f697c06 into bitcoin:master Jun 24, 2022
@theStack theStack deleted the 202206-txdb-refactor_remove_unused_getvaluesize_method branch June 24, 2022 14:55
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 27, 2022
Merge bitcoin#25438: refactor: remove unused methods in classes `CDBI…
e1fc0f6 
…terator,CDBWrapper,CCoinsViewDBCursor`

e4b4db5 refactor: remove unused method `CDBWrapper::CompactRange` (Sebastian Falbesoner)
fb38c6e refactor: remove unused methods `{CDBIterator,CCoinsViewDBCursor}::GetValueSize()` (Sebastian Falbesoner)

Pull request description:

  The `GetValueSize` methods haven't been used since the chainstate db cache has been switched from per-tx to per-txout model years ago (PR bitcoin#10195, commit d342424). The `CompactRange` is unused since the txindex migration code was removed (PR bitcoin#22626, commit bitcoin@fa20f81).

ACKs for top commit:
  fanquake:
    ACK e4b4db5
  furszy:
    re-ACK e4b4db5
  laanwj:
    Code review ACK e4b4db5

Tree-SHA512: 77da445fb70c744046263c6f2ddb05782b68e3d4b2ea604dd7c7dc73ce7c1f2d2b48ec68db4dcb03e35fc27488b99b0a420f6fa3d5b83d325c1708ed68e99e0a
@bitcoin bitcoin locked and limited conversation to collaborators Jun 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj approved these changes

@fanquake fanquake fanquake approved these changes

+3 more reviewers

@furszy furszy furszy approved these changes

@Riahiamirreza Riahiamirreza Riahiamirreza approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring UTXO Db and Indexes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@theStack @laanwj @furszy @maflcko @DrahtBot @fanquake @Riahiamirreza @w0xlt