Page MenuHomePhabricator

Abstract out the TxRequestTracker public interface
ClosedPublic

Authored by Fabien on May 21 2021, 20:35.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC8f3895fd9f2a: Abstract out the TxRequestTracker public interface
Summary

There is zero conceptual need for such an interface as there is no plan
to have different implementations, but this allows for declaring the
public methods in the header without having to pull all the internals
into the public space. This will become relevant when the
TxRequestTracker becomes a template.

Note: in order to instanciate the concrete implementation class without
knowing it, a static factory method is added to the base class. This is
unusual, as the base class is then responsible for instanciating its
child class, hardcoded (since there is a single one).

Depends on D9566.

Test Plan
ninja all check-all
./test/functional/test_runner.py p2p_tx_download
ninja bitcoin-fuzzers

Diff Detail

Event Timeline

Fabien requested review of this revision.May 21 2021, 20:35
Fabien planned changes to this revision.May 21 2021, 20:55
deadalnix requested changes to this revision.May 26 2021, 15:30
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/txrequest.cpp
357

implconcrete

861

Why not use make_unique ?

914

You made those protected, it doesn't seem to make a lot of sens to use them in this way.

src/txrequest.h
122

It doesn't sound like a name that stands on its own. Impl was previous scoped, so it somewhat made sense, but it doesn't now.

130

This most likely should be public.

132

Capitalize types.

139

Note that you almost certainly creates copies when you call these.

This revision now requires changes to proceed.May 26 2021, 15:30
Mengerian added a task: Restricted Maniphest Task.May 26 2021, 15:35
src/txrequest.cpp
861

make_unique cannot return a unique_ptr of <Impl> by building an ImplConcrete due to the way the template is defined

Make the interface methods public, capitalize the typedefs, pick better class names, grind the concrete

You might want to fix the Diff title... unless it really is a "pubic" interface 😂

deadalnix requested changes to this revision.May 26 2021, 21:01
deadalnix added inline comments.
src/txrequest.cpp
861

So?

https://godbolt.org/z/hGo51shnd

Unless I missed something, it works.

src/txrequest.h
157 ↗(On Diff #28619)

You also want to make it purely virtual so that this is an interface. You also want to to define it first, IMO. While not strictly necessary here, it is customary to do so in C++

This revision now requires changes to proceed.May 26 2021, 21:01
Fabien retitled this revision from Abstract out the TxRequestTracker pubic interface to Abstract out the TxRequestTracker public interface.May 27 2021, 06:56

Make the interface destructor purely virtual, use std::make_unique

src/txrequest.cpp
861

OK, no idea what I did to get a compiler error but I can't reproduce anymore so clearly I did something wrong. Will update.

src/txrequest.h
157 ↗(On Diff #28619)

TIL that you always need to define a pure virtual destructor even if this is a default destructor. What's funny is how incredibly helpful GCC is to hint you at the issue if you don't:

FAILED: src/bitcoind 
: && /usr/bin/g++ -g -O2 -fuse-ld=gold -Wl,-z,relro -Wl,-z,now -fPIE -pie src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o -o src/bitcoind  /usr/lib/libjemalloc_pic.a  src/libserver.a  src/leveldb/libleveldb.a  src/leveldb/libleveldb-sse4.2.a  src/leveldb/libmemenv.a  /usr/lib/libevent_pthreads.so  /usr/lib/libminiupnpc.so  src/wallet/libwallet.a  src/libbitcoinconsensus.a  src/libscript.a  src/libcommon.a  src/libbitcoinconsensus.a  src/libscript.a  src/libcommon.a  src/secp256k1/libsecp256k1.a  /usr/lib/libdb_cxx.so  src/zmq/libzmq.a  src/libutil.a  /usr/lib/libevent.so  src/univalue/libunivalue.a  src/crypto/libcrypto.a  src/crypto/libcrypto_sse4.1.a  src/crypto/libcrypto_avx2.a  src/crypto/libcrypto_shani.a  /usr/lib/libjemalloc_pic.a  -lm  -ldl  /usr/lib/libboost_filesystem.so.1.75.0  /usr/lib/libboost_thread.so.1.75.0  -pthread  /usr/lib/libzmq.so && :
/usr/bin/ld.gold : erreur interne dans format_file_lineno, à la position /build/binutils/src/binutils-gdb/gold/dwarf_reader.cc : 2278
collect2: erreur: ld a retourné le statut de sortie 1
deadalnix requested changes to this revision.May 27 2021, 13:38
deadalnix added inline comments.
src/txrequest.cpp
862 ↗(On Diff #28628)

That is not useful. Remove.

864 ↗(On Diff #28628)

Why do you need a function that just call another function? Why don't you call that other function right away?

This revision now requires changes to proceed.May 27 2021, 13:38
src/txrequest.cpp
864 ↗(On Diff #28628)

When TxRequestTracker is made a template, the constructor definition needs to move to the header file and this prevents the header to know about the concrete implementation, only the interface needs to be declared.

Revert to "impur" virtual destructor with default

This revision is now accepted and ready to land.May 27 2021, 17:07