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

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.May 21 2021, 20:35

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 21 2021, 20:35

Fabien requested review of this revision.May 21 2021, 20:35

Harbormaster completed remote builds in B15778: Diff 28567.May 21 2021, 20:45

Fabien planned changes to this revision.May 21 2021, 20:55

Remvove commented code

Fabien mentioned this in D9563: Turn InvRequestTracker into a template class.May 21 2021, 21:05

Fabien added a child revision: D9563: Turn InvRequestTracker into a template class.May 21 2021, 21:06

Harbormaster completed remote builds in B15779: Diff 28568.May 21 2021, 21:12

deadalnix requested changes to this revision.May 26 2021, 15:30

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/txrequest.cpp
357 ↗	(On Diff #28568)
861 ↗	(On Diff #28568)	Why not use make_unique ?
914 ↗	(On Diff #28568)	You made those protected, it doesn't seem to make a lot of sens to use them in this way.
src/txrequest.h
122 ↗	(On Diff #28568)	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 ↗	(On Diff #28568)	This most likely should be public.
132 ↗	(On Diff #28568)	Capitalize types.
139 ↗	(On Diff #28568)	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

Fabien added inline comments.May 26 2021, 18:47

src/txrequest.cpp
861 ↗	(On Diff #28568)	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

Harbormaster completed remote builds in B15815: Diff 28619.May 26 2021, 19:04

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 ↗	(On Diff #28568)	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 ↗

(On Diff #28568)

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

Harbormaster completed remote builds in B15824: Diff 28628.May 27 2021, 08:07

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

Fabien added inline comments.May 27 2021, 14:34

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.