forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Txid type-safety refactors #3
Comments
dergoegge
pushed a commit
that referenced
this issue
Jul 29, 2024
The previous commit added a test which would fail the unsigned-integer-overflow sanitizer. The warning is harmless and can be triggered on any commit, since the code was introduced. For reference, the warning would happen when the separator `-` was not present. For example: GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json would result in: rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long') #0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77 #1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9 #3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13 #4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12 #5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2) #6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o bitcoin#7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) bitcoin#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We introduced type-safe transaction identifiers in bitcoin#28107 to avoid type confusion bugs between txids and wtxids. We are in the process of converting more of our code to use these new types. In the following sections, I am laying out some thoughts on areas that need changing:
blockencodings.{h, cpp}
In blockencodings.h you will find the definition of the class
CBlockHeaderAndShortTxIDs
, which represents the BIP 152 “cmpctblock” p2p message. Its methodGetShortID
converts a wtxid to a BIP 152 short id. At the moment, this method takes aconst uint256&
as a parameter which is not type-safe (e.g. one might incorrectly pass a txid and the compiler would not complain).Converting the method signature from
uint64_t GetShortID(const uint256& txhash) const
touint64_t GetShortID(const Wtxid& wtxid) const
would be one of the steps required to make the blockencodings interface type-safe w.r.t. to transaction identifiers.The second method that needs changing is
PartiallyDownloadedBlock::InitData
. One of its parameters is aconst std::vector<std::pair<uint256, CTransactionRef>>&
, representing a list of optimistically kept transactions that might be useful for block reconstruction. It is a vector of pairs containing a wtxid and a shared pointer to the corresponding transaction.One option to make this type-safe, is to change the signature to be
const std::vector<std::pair<Wtxid, CTransactionRef>>&
(notice theWtxid
type instead ofuint256
). A second option might arise from considering the following question: why are we passing pairs of (wtxid, tx) when the transaction itself is sufficient to retrieve the wtxid (i.etx->GetWitnessHash()
)? We might be able to just pass a vector of transactions instead of the pairs. Regardless of which option is chosen, changes to the type ofPeerManagerImpl::vExtraTxnForCompact
will have to be made.GenTxid
GenTxid
(a generic txid type) existed before the new txid types. It can hold either a txid or wtxid but it does not enforce types at compile time. It will likely be a bigger refactor but this type should probably become astd::variant<Txid, Wtxid>
. To start, one could replace the current definition of the class withusing GenTxid = std::variant<Txid, Wtxid>
and then compile to observe the errors (this should reveal all locations that need changing).The text was updated successfully, but these errors were encountered: