Skip to content

[DO NOT MERGE] Add tests for expand_getblockstats#5

Closed
jnewbery wants to merge 7 commits intomasterfrom
expand_getblockstats_0.1.1
Closed

[DO NOT MERGE] Add tests for expand_getblockstats#5
jnewbery wants to merge 7 commits intomasterfrom
expand_getblockstats_0.1.1

Conversation

@jnewbery
Copy link
Copy Markdown

@jnewbery jnewbery commented Jul 12, 2019

This branch adds tests and fixes a minor bug in the expand_getblockstats. Thanks to @bitschmidty
for providing the tests and the bug fix.

The commits in this branch should be the same as https://github.com/bitcoinops/bitcoin/releases/tag/0.1 except with the following differences:

  • I cherry-picked the test changes from Bitcoin PR 14802, with a couple of minor modifications (see commit message for details). Doing that means that it's very easy to rebase the test changes in these commits onto master.
  • test cases have been added (and json data files for the test updated)
  • TheSetHasKeys() calls for loop_inputs and loop_outputs has been updated in all commits.

@bitschmidty - the changes in this branch should be almost the same as in https://github.com/bitschmidty/bitcoin/tree/2019-06-rebase-expand-getblockstats, but back-applied to tag 0.1. There are some very minor differences in the test cases. I also needed to make a couple of additional modifications to the SetHasKeys() calls - you hadn't added all of the new stats to that call in the right commits.

@0xB10C - please review just the changes to the test cases and json data file.

Once we're happy with this I'll tag it as 0.1.1. We can then verify that the tests on the rebased version pass with the same data file input.

jnewbery and others added 2 commits July 12, 2019 11:30
@jnewbery
Test changes from PR 14802
b40877e 
Add these test changes, which remove the requirement for a second
test node, to make rebasing our tests easier.

Minor changes:
- enable txindex
- don't use get_deterministic_priv_key()
@jnewbery
Add functions to check for spent SegWit outputs
73f1c38 
Adds member functions to CTxIn and CScript for determining if a
transaction spends any of the SegWit V0 output types.
jnewbery
jnewbery commented Jul 12, 2019
View reviewed changes
Comment thread test/functional/rpc_getblockstats.py Outdated
return [self.nodes[0].getblockstats(hash_or_height=self.start_height + i) for i in range(self.max_stat_pos+1)]

def generate_test_data(self, filename):
node = self.nodes[0]
Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

I suggest you don't make this change to our patch set. If this file changes in master, then a change like this will make rebasing more difficult.

Comment thread test/functional/rpc_getblockstats.py Outdated
pk1 = node.getaddressinfo(node.getnewaddress())['pubkey']
pk2 = node.getaddressinfo(node.getnewaddress())['pubkey']

nested_p2wpkh_address = self.nodes[0].getnewaddress('', 'bech32')
Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

Can you move these changes to the first commit (Implement SegWit related stats in getblockstats)? That way the new statistics get tested in the same commit that they're introduced in.

Ideally we want each new statistic to be tested in the commit that introduces it. For Implement SegWit related stats in getblockstats, those statistics are:

  • nested_p2wpkh_outputs_spent
  • nested_p2wsh_outputs_spent
  • native_p2wpkh_outputs_spent
  • native_p2wsh_outputs_spent
  • txs_spending_nested_p2wpkh_outputs
  • txs_spending_nested_p2wsh_outputs
  • txs_spending_native_p2wpkh_outputs
  • txs_spending_native_p2wsh_outputs
  • new_p2wpkh_outputs
  • new_p2wsh_outputs
  • txs_creating_p2wpkh_outputs
  • txs_creating_p2wsh_outputs

so you want to create an address for each type (as you do here), and make sure funds are sent to those addresses and subsequently spent.

Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

I think the only type of transaction you've missed here is spending from a p2sh-p2wpkh. If you add that, I think you've triggered all the stats.

Copy link
Copy Markdown
Collaborator

@bitschmidty bitschmidty Jul 12, 2019

Choose a reason for hiding this comment

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

p2sh-p2wpkh was handled previously with:

nested_p2wpkh_address = node.get_deterministic_priv_key().address

Looks like you missed that in copying over from my branch. I will amend.

Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

get_deterministic_priv_key() doesn't work here because that function didn't exist when this branch was created, which is why I changed this.

I made a mistake though. We need to change:

nested_p2wpkh_address = self.nodes[0].getnewaddress('', 'bech32')

to be:

nested_p2wpkh_address = self.nodes[0].getnewaddress('', 'p2sh-segwit')

Comment thread test/functional/rpc_getblockstats.py Outdated

self.nodes[0].sendtoaddress(address=address, amount=10, subtractfeefromamount=True)
self.nodes[0].sendtoaddress(address=address, amount=10, subtractfeefromamount=False)
self.nodes[0].sendtoaddress(address=address, amount=0.00001, subtractfeefromamount=False)
Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

Nice way to trigger the dust statistics!

Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

Can you add a comment to say that this amount is to trigger the dust statistics.

jnewbery
jnewbery commented Jul 12, 2019
View reviewed changes
Comment thread test/functional/rpc_getblockstats.py Outdated

inputs = self.nodes[0].listunspent()[:3]
outputs = {self.nodes[0].getnewaddress():1,self.nodes[0].getnewaddress():2,self.nodes[0].getnewaddress():sum(i["amount"] for i in inputs)-Decimal("3.001")}
rawtx = self.nodes[0].createrawtransaction(inputs, outputs, 0, True)
Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

Can you use named arguments here (createrawtransaction(inputs=inputs, outputs=outputs, locktime=0, replaceable=True)) so it's clear that this tx is signaling opt-in RBF. Maybe also add a comment above saying that this tx is signaling opt-in RBF.

jnewbery
jnewbery commented Jul 12, 2019
View reviewed changes
Comment thread test/functional/rpc_getblockstats.py Outdated
self.nodes[0].sendtoaddress(address=address, amount=10, subtractfeefromamount=True)

inputs = self.nodes[0].listunspent()[:3]
outputs = {self.nodes[0].getnewaddress():1,self.nodes[0].getnewaddress():2,self.nodes[0].getnewaddress():sum(i["amount"] for i in inputs)-Decimal("3.001")}
Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

Add a comment to say that this is a batch transaction.

Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

Also need to create a consolidation transaction.

jnewbery
jnewbery commented Jul 12, 2019
View reviewed changes
Comment thread test/functional/rpc_getblockstats.py Outdated

inputs = self.nodes[0].listunspent()[:3]
outputs = {self.nodes[0].getnewaddress():1,self.nodes[0].getnewaddress():2,self.nodes[0].getnewaddress():sum(i["amount"] for i in inputs)-Decimal("3.001")}
outputs = {self.nodes[0].getnewaddress():sum(i["amount"] for i in inputs)-Decimal("0.001")}
Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 12, 2019

Choose a reason for hiding this comment

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

I think these changes to the test file can go in the previous commit (Add more stats to getblockstats) so the consolidation transaction stat is triggered in that commit.

@bitschmidty bitschmidty force-pushed the expand_getblockstats_0.1.1 branch from b08dd7f to 6b2e3eb Compare July 12, 2019 17:55
@bitschmidty
Copy link
Copy Markdown
Collaborator

bitschmidty commented Jul 12, 2019

@jnewbery force pushed a commit with the suggestions you outlined

0xB10C
0xB10C reviewed Jul 14, 2019
View reviewed changes
Copy link
Copy Markdown

@0xB10C 0xB10C left a comment

Choose a reason for hiding this comment

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

  • ran and looked over the rpc_getblockstats.py test
  • looked over the additions in rpc_getblockstats.json and compared to the corresponding lines in rpc_getblockstats.py with the help of the getblock output for both blocks

Comment thread test/functional/rpc_getblockstats.py Outdated
nested_p2wsh_address = self.nodes[0].addmultisigaddress(2, [pk1,pk2], '', 'p2sh-segwit')['address']

# testing batching, nested p2wpkh, dust, native p2wsh, and nested p2wsh metrics
outputs = {nested_p2wpkh_address:4,self.nodes[0].getnewaddress('', 'legacy'):0.00001,native_p2wsh_address:2,nested_p2wsh_address:30}
Copy link
Copy Markdown

@0xB10C 0xB10C Jul 14, 2019

Choose a reason for hiding this comment

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

nit: Add a space or line break after each argument to improve readability.

Copy link
Copy Markdown

@0xB10C 0xB10C Jul 14, 2019

Choose a reason for hiding this comment

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

We create two p2wsh outputs: one native and one nested. In the JSON file only one is counted in new_p2wsh_outputs.

bitcoin/test/functional/data/rpc_getblockstats.json

Line 260 in 6b2e3eb

"new_p2wsh_outputs": 1,

I guess what happens here is that we can't determine if a p2sh wraps a p2w* script from the scriptPubKey. Maybe consider renaming both new_p2wpkh_outputs and new_p2wsh_outputs to new_native_p2w(pkh/sh)_outputs to avoid confusion somewhere down the line.

Copy link
Copy Markdown
Author

@jnewbery jnewbery Jul 16, 2019

Choose a reason for hiding this comment

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

nit: Add a space or line break after each argument to improve readability.

Done

Maybe consider renaming...

That's a good suggestion. However, it'd be a breaking API change, and we already have data on our deployed dashboard that uses the current naming.

I'll keep it as it is, but update the help text to make it clear that this is just for the native outputs.

Marcin Jachymiak added 2 commits July 16, 2019 15:49
@jnewbery
Implement SegWit related stats in getblockstats
402868d 
Count creation of new native P2WSH and P2WPKH outputs, and the number of
transactions creating each type of output.

Count the number of native P2WPKH, native P2WSH, nested P2WSH, and
nested P2WPKH outputs spent. Count the number of transactions that spend
each type of SegWit output.

Test changes contributed by Mike Schmidt <schmidty@gmail.com>.
@jnewbery
Add stats for creation of dust outputs
655d385 
For each new output, checks whether it would be considered dust at
several different fee-rates.

Test changes contributed by Mike Schmidt <schmidty@gmail.com>.
@jnewbery jnewbery force-pushed the expand_getblockstats_0.1.1 branch from 6b2e3eb to abb58f5 Compare July 16, 2019 20:00
Marcin Jachymiak added 3 commits July 16, 2019 17:38
@jnewbery
Add more stats to getblockstats
840f2b5 
Adds batching transaction counter.
Tracks the number of outputs created by each transaction.
Adds consolidating transaction counter.
Counts the number of transactions signalling opt-in RBF.

Test changes contributed by Mike Schmidt <schmidty@gmail.com>.
@jnewbery
Track many-to-one consolidations
3592649 
getblockstats now counts the number of transactions with at least 3
inputs and exactly one output as "many-to-one" (mto) consolidations.

It also counts the total number of outputs consolidated by mto
transactions per block.

Test changes contributed by Mike Schmidt <schmidty@gmail.com>.
@jnewbery
Add value tracking stats
1a559e4 
Tracks total value of many_to_one outputs.
Tracks total value of different kinds of SegWit outputs spent.
Tracks total value of native SegWit outputs created.

Test changes contributed by Mike Schmidt <schmidty@gmail.com>.
@jnewbery jnewbery force-pushed the expand_getblockstats_0.1.1 branch from abb58f5 to 1a559e4 Compare July 16, 2019 21:41
@jnewbery
Copy link
Copy Markdown
Author

jnewbery commented Jul 16, 2019

Thanks @bitschmidty . This looks great. I've tagged it as release 0.1.1: https://github.com/bitcoinops/bitcoin/releases/tag/0.1.1

@jnewbery jnewbery closed this Jul 16, 2019
@0xB10C 0xB10C mentioned this pull request Jul 16, 2019
Closed
elichai pushed a commit that referenced this pull request Jan 14, 2020
@laanwj
Merge bitcoin#17427: qt: Fix missing qRegisterMetaType for size_t
89e9313 
1828c6f refactor: Styling w/ clang-format, comment update (Hennadii Stepanov)
88a94f7 qt: Fix missing qRegisterMetaType for size_t (Hennadii Stepanov)

Pull request description:

  On master (a7aec7a) this connection https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/rpcconsole.cpp#L587 fails due to `ClientModel::mempoolSizeChanged()` signal has unregistered parameter type `size_t`: https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/clientmodel.h#L102

  More:
  ```
  $ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- -debug=qt
  ...
  (lldb) bt
  * thread bitcoin#17, name = 'QThread', stop reason = signal SIGABRT
    * frame #0: 0x00007ffff35fce97 libc.so.6`__GI_raise(sig=2) at raise.c:51
      frame #1: 0x00007ffff35fe801 libc.so.6`__GI_abort at abort.c:79
      frame #2: 0x00007ffff5901352 libQt5Core.so.5`QMessageLogger::warning(char const*, ...) const + 354
      frame #3: 0x00007ffff5b216fe libQt5Core.so.5`___lldb_unnamed_symbol2329$$libQt5Core.so.5 + 334
      frame #4: 0x00007ffff5b2456d libQt5Core.so.5`QMetaObject::activate(QObject*, int, int, void**) + 1933
      frame #5: 0x000055555566872e bitcoin-qt`ClientModel::mempoolSizeChanged(this=<unavailable>, _t1=<unavailable>, _t2=<unavailable>) at moc_clientmodel.cpp:260
  ...

  ```

  `debug.log`:
  ```
  [] GUI: QObject::connect: Cannot queue arguments of type 'size_t'
  (Make sure 'size_t' is registered using qRegisterMetaType().)
  ```

  This PR fixes it.

  Refs:
  - [Qt docs: qRegisterMetaType](https://doc.qt.io/qt-5/qmetatype.html#qRegisterMetaType)
  - bitcoin#16348

  ---

  Side NOTE: Also I believe this line https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/bitcoin.cpp#L63 is redundant since long `CAmount` is a `typedef`.

ACKs for top commit:
  laanwj:
    Tested ACK 1828c6f

Tree-SHA512: 2c7f9fe6a5ae70f2e1dd86b07f95d4b00c85c5706a9d722f063f80beb71880d012ec46556963fb1544c2af53d006936c2f7612eae60d9193f67db62ba3d86129
remyers pushed a commit to remyers/bitcoin that referenced this pull request Oct 3, 2021
Merge bitcoin#21773: fuzz: Ensure prevout is consensus-valid
bce09da 
fa1fdeb fuzz: Ensure prevout is consensus-valid (MarcoFalke)

Pull request description:

  It shouldn't be possible to create consensus-invalid prevouts, so there is no need to fuzz them.

  To reproduce:

  ```
  $ echo 'AAAAAAEAAAAAAQF0Rw0SGsrit4+YZSEfpcQT/o+bJbjgVjATUHqrCfRE+QsBAAAXFgAUlsvXHgGV
  ZxF3QXxitwe1tIOYdLj2NePHATl9CgAAAAAAGXapFOFHg1yqRFl7soeowwpIEOoe9G1NiKwCRzBE
  AiAx6F2Q008gvJnok6JiyOn7lPqCJJmDiI2omRNXT1Q7XAIgCQP6WJizAqhnvImpQqYMJkqePGvx
  Jy/pGRMy1iNL0ecDIQJr4tWomVTBfjpyMFMOD9aDAR5gkByOIYiaQOv8P/sRztP3pS8RDAAAEUUE
  NQBwYAAAAAC5F6kUTLIzj/lKP2Hmpwyzukns2eweRkOH' | base64 --decode  > /tmp/a

  $ FUZZ=script_flags ./src/test/fuzz/fuzz /tmp/a
  INFO: Running with entropic power schedule (0xFF, 100).
  INFO: Seed: 59714236
  INFO: Loaded 1 modules   (212532 inline 8-bit counters): 212532 [0x55987fb3f668, 0x55987fb7349c),
  INFO: Loaded 1 PC tables (212532 PCs): 212532 [0x55987fb734a0,0x55987feb17e0),
  ./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
  Running: /tmp/a
  fuzz: script/interpreter.cpp:1495: bool HandleMissingData(MissingDataBehavior): Assertion `!"Missing data"' failed.
  ==520092== ERROR: libFuzzer: deadly signal
      #0 0x55987f111180 in __sanitizer_print_stack_trace (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5ea180)
      #1 0x55987f0ba828 in fuzzer::PrintStackTrace() fuzzer.o
      #2 0x55987f09de43 in fuzzer::Fuzzer::CrashCallback() fuzzer.o
      bitcoinops#3 0x7fd003d563bf  (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf)
      bitcoinops#4 0x7fd00399a18a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a)
      bitcoinops#5 0x7fd003979858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858)
      bitcoinops#6 0x7fd003979728  (/lib/x86_64-linux-gnu/libc.so.6+0x25728)
      bitcoinops#7 0x7fd00398af35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35)
      bitcoinops#8 0x55987f8ce194 in HandleMissingData(MissingDataBehavior) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1495:9
      bitcoinops#9 0x55987f8ce194 in GenericTransactionSignatureChecker<CTransaction>::CheckECDSASignature(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, CScript const&, SigVersion) const /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1685:68
      bitcoinops#10 0x55987f8cbbc8 in EvalChecksigPreTapscript(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, prevector<28u, unsigned char, unsigned int, int>::const_iterator, prevector<28u, unsigned char, unsigned int, int>::const_iterator, unsigned int, BaseSignatureChecker const&, SigVersion, ScriptError_t*, bool&) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:363:24
      bitcoinops#11 0x55987f8cbbc8 in EvalChecksig(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, prevector<28u, unsigned char, unsigned int, int>::const_iterator, prevector<28u, unsigned char, unsigned int, int>::const_iterator, ScriptExecutionData&, unsigned int, BaseSignatureChecker const&, SigVersion, ScriptError_t*, bool&) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:421:16
      bitcoinops#12 0x55987f8c5a01 in EvalScript(std::vector<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >&, CScript const&, unsigned int, BaseSignatureChecker const&, SigVersion, ScriptExecutionData&, ScriptError_t*) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1094:26
      bitcoinops#13 0x55987f8d6d6e in ExecuteWitnessScript(Span<std::vector<unsigned char, std::allocator<unsigned char> > const> const&, CScript const&, unsigned int, SigVersion, BaseSignatureChecker const&, ScriptExecutionData&, ScriptError_t*) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1843:10
      bitcoinops#14 0x55987f8d48fc in VerifyWitnessProgram(CScriptWitness const&, int, std::vector<unsigned char, std::allocator<unsigned char> > const&, unsigned int, BaseSignatureChecker const&, ScriptError_t*, bool) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:1904:20
      bitcoinops#15 0x55987f8d3d8b in VerifyScript(CScript const&, CScript const&, CScriptWitness const*, unsigned int, BaseSignatureChecker const&, ScriptError_t*) /root/fuzz_dir/scratch/fuzz_gen/code/src/script/interpreter.cpp:2045:22
      bitcoinops#16 0x55987f201d47 in script_flags_fuzz_target(Span<unsigned char const>) /root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/script_flags.cpp:54:30
      bitcoin#17 0x55987f11447f in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2
      bitcoin#18 0x55987f8aed17 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      bitcoin#19 0x55987f8aed17 in LLVMFuzzerTestOneInput /root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz.cpp:63:5
      bitcoin#20 0x55987f09f5e3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
      bitcoin#21 0x55987f0894e2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
      bitcoin#22 0x55987f08f2da in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
      bitcoin#23 0x55987f0bb002 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x594002)
      bitcoin#24 0x7fd00397b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
      bitcoin#25 0x55987f06420d in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x53d20d)

  NOTE: libFuzzer has rudimentary signal handlers.
        Combine libFuzzer with AddressSanitizer or similar for better crash reports.
  SUMMARY: libFuzzer: deadly signal

ACKs for top commit:
  practicalswift:
    Tested ACK fa1fdeb

Tree-SHA512: 6130ed9ab6d8eeab901f64a1c069300e67d0b6009c42763262fe6edeab8192e088c1a3c1f61aee900b9ebbc48fbf6e837b41704bad592ec526398355766e208a
remyers pushed a commit to remyers/bitcoin that referenced this pull request Oct 3, 2021
Merge bitcoin#21802: refactor: Avoid UB in util/asmap (advance a dere…
eb9a1fe 
…ferenceable iterator outside its valid range)

fa09871 refactor: Avoid sign-compare compiler warning in util/asmap (MarcoFalke)

Pull request description:

  Can be reproduced on current master with `D_GLIBCXX_DEBUG`:

  ```
  /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/debug/safe_iterator.h:883:
  In function:
      __gnu_debug::_Safe_iterator<type-parameter-0-0, type-parameter-0-1,
      std::random_access_iterator_tag>::_Self __gnu_debug::operator+(const
      __gnu_debug::_Safe_iterator<type-parameter-0-0, type-parameter-0-1,
      std::random_access_iterator_tag>::_Self &,
      __gnu_debug::_Safe_iterator<type-parameter-0-0, type-parameter-0-1,
      std::random_access_iterator_tag>::difference_type)

  Error: attempt to advance a dereferenceable iterator 369 steps, which falls
  outside its valid range.

  Objects involved in the operation:
      iterator @ 0x0x7ffd3d613138 {
        type = std::__cxx1998::_Bit_const_iterator (constant iterator);
        state = dereferenceable;
        references sequence with type 'std::__debug::vector<bool, std::allocator<bool> >' @ 0x0x7ffd3d663590
      }
  ==65050== ERROR: libFuzzer: deadly signal
      #0 0x559ab9787690 in __sanitizer_print_stack_trace (/bitcoin/src/test/fuzz/fuzz+0x5a1690)
      #1 0x559ab9733998 in fuzzer::PrintStackTrace() (/bitcoin/src/test/fuzz/fuzz+0x54d998)
      #2 0x559ab9718ae3 in fuzzer::Fuzzer::CrashCallback() (/bitcoin/src/test/fuzz/fuzz+0x532ae3)
      bitcoinops#3 0x7f70a0e723bf  (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf)
      bitcoinops#4 0x7f70a0b3418a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a)
      bitcoinops#5 0x7f70a0b13858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858)
      bitcoinops#6 0x7f70a0f21148  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xa1148)
      bitcoinops#7 0x559ab9f60a96 in __gnu_debug::operator+(__gnu_debug::_Safe_iterator<std::__cxx1998::_Bit_const_iterator, std::__debug::vector<bool, std::allocator<bool> >, std::random_access_iterator_tag> const&, long) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/debug/safe_iterator.h:881:2
      bitcoinops#8 0x559ab9f61062 in SanityCheckASMap(std::__debug::vector<bool, std::allocator<bool> > const&, int) util/asmap.cpp:159:21
      bitcoinops#9 0x559ab9e4fdfa in SanityCheckASMap(std::__debug::vector<bool, std::allocator<bool> > const&) netaddress.cpp:1242:12
      bitcoinops#10 0x559ab9793fcb in addrman_fuzz_target(Span<unsigned char const>) test/fuzz/addrman.cpp:43:14
      bitcoinops#11 0x559ab978a03c in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2
      bitcoinops#12 0x559aba2692c7 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      bitcoinops#13 0x559aba269132 in LLVMFuzzerTestOneInput test/fuzz/fuzz.cpp:63:5
      bitcoinops#14 0x559ab971a1a1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/bitcoin/src/test/fuzz/fuzz+0x5341a1)
      bitcoinops#15 0x559ab97198e5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/bitcoin/src/test/fuzz/fuzz+0x5338e5)
      bitcoinops#16 0x559ab971bb87 in fuzzer::Fuzzer::MutateAndTestOne() (/bitcoin/src/test/fuzz/fuzz+0x535b87)
      bitcoin#17 0x559ab971c885 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/bitcoin/src/test/fuzz/fuzz+0x536885)
      bitcoin#18 0x559ab970b23e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/bitcoin/src/test/fuzz/fuzz+0x52523e)
      bitcoin#19 0x559ab9734082 in main (/bitcoin/src/test/fuzz/fuzz+0x54e082)
      bitcoin#20 0x7f70a0b150b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
      bitcoin#21 0x559ab96dffdd in _start (/bitcoin/src/test/fuzz/fuzz+0x4f9fdd)

ACKs for top commit:
  sipa:
    utACK fa09871
  vasild:
    ACK fa09871

Tree-SHA512: 802fda33bda40fe2521f1e3be075ceddc5fd9ba185bd494286e50019931dfd688da7a6513601138b1dc7bb8e80ae47c8572902406eb59f68990619ddb2656748
remyers pushed a commit to remyers/bitcoin that referenced this pull request Oct 3, 2021
@laanwj
Merge bitcoin#21584: Fix assumeutxo crash due to invalid base_blockhash
ee9befe 
fa340b8 refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash (MarcoFalke)
fae33f9 Fix assumeutxo crash due to invalid base_blockhash (MarcoFalke)
fa5668b refactor: Use type-safe assumeutxo hash (MarcoFalke)
0000007 refactor: Remove unused code (MarcoFalke)
faa921f move-only: Add util/hash_type (MarcoFalke)

Pull request description:

  Starting with commit d6af06d, a block hash of all-zeros is invalid and will lead to a crash of the node. Can be tested by cherry-picking the test changes without the other changes.

  Stack trace (copied from bitcoin#21584 (comment)):

  ```
  #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
  #1  0x00007ffff583c8b1 in __GI_abort () at abort.c:79
  #2  0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
      assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89,
      function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
  bitcoinops#3  0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89,
      function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
  bitcoinops#4  0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
  bitcoinops#5  0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
  bitcoinops#6  0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
  bitcoinops#7  0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
      at validation.cpp:5422
  bitcoinops#8  0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
  bitcoinops#9  0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=...,
      root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
  bitcoinops#10 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
      at test/validation_chainstatemanager_tests.cpp:262

ACKs for top commit:
  laanwj:
    Code review re-ACK fa340b8
  jamesob:
    ACK fa340b8 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))

Tree-SHA512: c2c4e66c1abfd400ef18a04f22fec1f302f1ff4d27a18050f492f688319deb4ccdd165ff792eee0a1f816e7b69fb64080662b79517ab669e3d26b9eb77802851
remyers pushed a commit to remyers/bitcoin that referenced this pull request Oct 3, 2021
Merge bitcoin#21948: test: Fix off-by-one in mockscheduler test RPC
c857148 
fa2e614 test: Fix off-by-one in mockscheduler test RPC (MarcoFalke)

Pull request description:

  Fixes:

  ```
  fuzz: scheduler.cpp:83: void CScheduler::MockForward(std::chrono::seconds): Assertion `delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}' failed.
  ==1059066== ERROR: libFuzzer: deadly signal
      #0 0x558f75449c10 in __sanitizer_print_stack_trace (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5fec10)
      #1 0x558f753f32b8 in fuzzer::PrintStackTrace() fuzzer.o
      #2 0x558f753d68d3 in fuzzer::Fuzzer::CrashCallback() fuzzer.o
      bitcoinops#3 0x7f4a3cbbb3bf  (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf)
      bitcoinops#4 0x7f4a3c7ff18a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a)
      bitcoinops#5 0x7f4a3c7de858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858)
      bitcoinops#6 0x7f4a3c7de728  (/lib/x86_64-linux-gnu/libc.so.6+0x25728)
      bitcoinops#7 0x7f4a3c7eff35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35)
      bitcoinops#8 0x558f7588a913 in CScheduler::MockForward(std::chrono::duration<long, std::ratio<1l, 1l> >) scheduler.cpp:83:5
      bitcoinops#9 0x558f75b0e5b1 in mockscheduler()::$_7::operator()(RPCHelpMan const&, JSONRPCRequest const&) const rpc/misc.cpp:435:30
      bitcoinops#10 0x558f75b0e5b1 in std::_Function_handler<UniValue (RPCHelpMan const&, JSONRPCRequest const&), mockscheduler()::$_7>::_M_invoke(std::_Any_data const&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
      bitcoinops#11 0x558f7587a141 in std::function<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      bitcoinops#12 0x558f7587a141 in RPCHelpMan::HandleRequest(JSONRPCRequest const&) const rpc/util.cpp:565:26
      bitcoinops#13 0x558f756c0086 in CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const ./rpc/server.h:110:91
      bitcoinops#14 0x558f756c0086 in std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
      bitcoinops#15 0x558f756b8592 in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      bitcoinops#16 0x558f756b8592 in ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) rpc/server.cpp:480:20
      bitcoin#17 0x558f756b8592 in ExecuteCommands(std::vector<CRPCCommand const*, std::allocator<CRPCCommand const*> > const&, JSONRPCRequest const&, UniValue&) rpc/server.cpp:444:13
      bitcoin#18 0x558f756b8017 in CRPCTable::execute(JSONRPCRequest const&) const rpc/server.cpp:464:13
      bitcoin#19 0x558f7552457a in (anonymous namespace)::RPCFuzzTestingSetup::CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) test/fuzz/rpc.cpp:50:25
      bitcoin#20 0x558f7552457a in rpc_fuzz_target(Span<unsigned char const>) test/fuzz/rpc.cpp:354:28
      bitcoin#21 0x558f7544cf0f in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2
      bitcoin#22 0x558f75c05197 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      bitcoin#23 0x558f75c05197 in LLVMFuzzerTestOneInput test/fuzz/fuzz.cpp:74:5
      bitcoin#24 0x558f753d8073 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
      bitcoin#25 0x558f753c1f72 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
      bitcoin#26 0x558f753c7d6a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
      bitcoin#27 0x558f753f3a92 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5a8a92)
      bitcoin#28 0x7f4a3c7e00b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
      bitcoin#29 0x558f7539cc9d in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x551c9d)

ACKs for top commit:
  practicalswift:
    cr ACK fa2e614

Tree-SHA512: cfa120265261f0ad019b46c426b915c1c007806b37aecb27016ce780a0ddea5e6fc9b09065fd40684b11183dcd3bf543558d7a655e604695021653540266baf7
remyers pushed a commit to remyers/bitcoin that referenced this pull request Feb 12, 2023
@fanquake
Merge bitcoin#26477: validation: fix broken maxtipage comparison
968f03e 
e4be0e9 test: add -maxtipage test for the maximum allowable value (James O'Beirne)
a451e83 fix: validation: cast now() to seconds for maxtipage comparison (James O'Beirne)

Pull request description:

  Since bitcoin@faf4487, the maxtipage comparison in IsInitialBlockDownload() has been broken, since the NodeClock::now() time_point is in the system's native denomination (nanoseconds).

  Without this patch, specifying the maximum allowable -maxtipage (9223372036854775807) results in a SIGABRT crash:

  ```
  % gdb --args ./src/bitcoind -maxtipage=9223372036854775207 -minimumchainwork=0x00 -stopatheight=30000
  ...
  2022-11-09T15:55:17Z [dnsseed] dnsseed thread exit
  [Thread 0x7fff937fe640 (LWP 69883) exited]

  Thread 29 "b-msghand" received signal SIGABRT, Aborted.
  [Switching to Thread 0x7fff91ffb640 (LWP 69886)]
  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
  44      ./nptl/pthread_kill.c: No such file or directory.
  (gdb) bt
  #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
  #1  0x00007ffff768989f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
  #2  0x00007ffff763da52 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
  bitcoinops#3  0x00007ffff7628469 in __GI_abort () at ./stdlib/abort.c:79
  bitcoinops#4  0x00007ffff7cf79a4 in __mulvdi3 () from /lib/x86_64-linux-gnu/libgcc_s.so.1
  bitcoinops#5  0x00005555558d13ab in std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, std::ratio<1000000000l, 1l>, long, false, true>::__cast<long, std::ratio<1l, 1l> > (__d=...) at /usr/include/c++/12/bits/chrono.h:521
  bitcoinops#6  std::chrono::duration_cast<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__d=...)
      at /usr/include/c++/12/bits/chrono.h:260
  bitcoinops#7  std::chrono::duration<long, std::ratio<1l, 1000000000l> >::duration<long, std::ratio<1l, 1l>, void> (__d=..., this=<optimized out>)
      at /usr/include/c++/12/bits/chrono.h:514
  bitcoinops#8  std::chrono::operator-<long, std::ratio<1l, 1000000000l>, long, std::ratio<1l, 1l> > (__rhs=..., __lhs=...)
      at /usr/include/c++/12/bits/chrono.h:650
  bitcoinops#9  std::chrono::operator-<NodeClock, std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__rhs=...,
      __lhs=...) at /usr/include/c++/12/bits/chrono.h:1020
  bitcoinops#10 Chainstate::IsInitialBlockDownload (this=0x555556071940) at ./src/validation.cpp:1545
  bitcoinops#11 0x00005555556efd1e in operator() (__closure=<optimized out>) at ./src/net_processing.cpp:3369
  bitcoinops#12 (anonymous namespace)::PeerManagerImpl::ProcessMessage (this=0x555556219be0, pfrom=..., msg_type=..., vRecv=..., time_received=...,
      interruptMsgProc=...) at ./src/net_processing.cpp:3369
  bitcoinops#13 0x00005555556f75cc in (anonymous namespace)::PeerManagerImpl::ProcessMessages (this=0x555556219be0, pfrom=<optimized out>,
      interruptMsgProc=std::atomic<bool> = { false }) at ./src/net_processing.cpp:4985
  bitcoinops#14 0x00005555556a83c9 in CConnman::ThreadMessageHandler (this=0x5555560ebc70) at ./src/net.cpp:2014
  bitcoinops#15 0x0000555555c4d5d6 in std::function<void ()>::operator()() const (this=0x7fff91ffadb0) at /usr/include/c++/12/bits/std_function.h:591
  bitcoinops#16 util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) (
      thread_name="0\255\377\221\377\177\000\000\v\000\000\000\000\000\000\000TraceThread\000\000\000\000\000P\255\377\221\377\177\000\000\017\000\000\000\000\000\000\000util/thread.cpp\000\000\000\000\000\000\000\000\000\000ihB鵿6\000\000\000\000\000\000\000\000\260\255\377\221\377\177\000\000\277\211\321UUU\000\000p\324\304UUU\000\000\002\000\000\000\000\000\000\000\240xh\367\377\177\000\000\000\000\000\000\000\000\000\000]\340iUUU\000\000p\274\016VUU\000\000\000\000\000\000\000\000\000\000\300\303iUUU\000\000p\206jUUU", '\000' <repeats 11 times>, "ihB鵿6\200\251!VUU\000\000"..., thread_func=...) at util/thread.cpp:21
  bitcoin#17 0x000055555569e05d in std::__invoke_impl<void, void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__f=<optimized out>) at /usr/include/c++/12/bits/invoke.h:61
  bitcoin#18 std::__invoke<void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__fn=<optimized out>) at /usr/include/c++/12/bits/invoke.h:96
  bitcoin#19 std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::_M_invoke<0, 1, 2> (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:252
  bitcoin#20 std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::operator() (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:259
  bitcoin#21 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > > >::_M_run(void) (this=<optimized out>)
      at /usr/include/c++/12/bits/std_thread.h:210
  bitcoin#22 0x00007ffff7ad43d3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
  bitcoin#23 0x00007ffff7687b27 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:435
  bitcoin#24 0x00007ffff770a78c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
  (gdb)
  ```

ACKs for top commit:
  MarcoFalke:
    review ACK e4be0e9 🏽

Tree-SHA512: d892d6264a284d952a68a8631a6301277373b8df939dafd9e2652f2f22ab60712cde63b90c27c67ea2d05f02443452e3e4e1b9f25479bfaca00d4c4de13b9fbd
remyers pushed a commit to remyers/bitcoin that referenced this pull request Jan 11, 2024
@fanquake
Merge bitcoin#28966: test: Add missing CBlockPolicyEstimator::process…
453c9ca 
…BlockTx suppression

fa9dc92 test: Add missing CBlockPolicyEstimator::processBlockTx suppression (MarcoFalke)

Pull request description:

  Fixes bitcoin#28865 (comment)

  ```
  # FUZZ=policy_estimator UBSAN_OPTIONS="suppressions=/root/fuzz_dir/scratch/fuzz_gen/code/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06

  ...

  policy/fees.cpp:632:27: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed)
      #0 0x55cbbe10daee in CBlockPolicyEstimator::processBlockTx(unsigned int, CTxMemPoolEntry const*) src/policy/fees.cpp:632:27
      #1 0x55cbbe10e361 in CBlockPolicyEstimator::processBlock(unsigned int, std::vector<CTxMemPoolEntry const*, std::allocator<CTxMemPoolEntry const*>>&) src/policy/fees.cpp:680:13
      #2 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:69:40
      bitcoinops#3 0x55cbbd84af48 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:43:27
      bitcoinops#4 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9
      bitcoinops#5 0x55cbbda1cc18 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
      bitcoinops#6 0x55cbbda1cc18 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
      bitcoinops#7 0x55cbbd26a944 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x190e944) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      bitcoinops#8 0x55cbbd253916 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f7916) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      bitcoinops#9 0x55cbbd25945a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18fd45a) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      bitcoinops#10 0x55cbbd284026 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x1928026) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      bitcoinops#11 0x7fe4aa8280cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      bitcoinops#12 0x7fe4aa828188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      bitcoinops#13 0x55cbbd24e494 in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f2494) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)

  SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change policy/fees.cpp:632:27 in
  ```

  ```
  # base64 /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06
  AQEAAAAAADkFlVwAAQEAAAAAADkFlZVcACTDSSsP3746IAZrH48khwMAAQEB/QEALQAACwAAAAAA
  FgAAAAAAAQAABgAAAAAAAAAAAAAAAAAAACcQAAAAAAAAAAAAAAAAAAAAAAD6AAAAOQWVXAABAQAA
  AAAAOQWVlVwAIMNJKw/fvjogBmsfjySHAwABAQH9AQAtAAALAAAAAAAAAAABAAAGAAAAAAAAAAAA
  AAAAAAAAJxAAAAAAAAAAAAAAAAAAAAAAAPr/AAAAAAAAAAAAAAQAAAAA/wAAAAAAAAAAAAAEAAAA
  AAEBAeAIAVwBXAAA/jbSBvwBKABSKBwBYgEB2wAEkvXInHYAAAAAAAAAvgAAAAAA/9//6v8e/xIk
  MgAlAiUAOw==

ACKs for top commit:
  fanquake:
    ACK fa9dc92
  dergoegge:
    utACK fa9dc92

Tree-SHA512: 3898c17c928ecc2bcc8c7086359e9ae00da2197b4d8e10c7bf6d12415326c9bca3ef6e1d8d3b83172ccfa604ce7e7371415262ba705225f9ea4da8b1a7eb0306
remyers pushed a commit to remyers/bitcoin that referenced this pull request Jan 11, 2024
@fanquake
Merge bitcoin#28969: fuzz: Avoid signed-integer-overflow in wallet_no…
7bc8c53 
…tifications fuzz target

fab164f fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke)

Pull request description:

  Should avoid

  ```
  policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long')
      #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63
      #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional<CFeeRate>) src/./wallet/coinselection.h:91:57
      #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17
      bitcoinops#3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33
      bitcoinops#4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16
      bitcoinops#5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set<int, std::less<int>, std::allocator<int>> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16
      bitcoinops#6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15
      bitcoinops#7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23
      bitcoinops#8 0x563a15958240 in unsigned long CallOneOf<wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1) src/./test/fuzz/util.h:43:27
      bitcoinops#9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/notifications.cpp:196:9
      bitcoinops#10 0x563a15fdef0c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
      bitcoinops#11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
      bitcoinops#12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      bitcoinops#13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      bitcoinops#14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      bitcoinops#15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      bitcoinops#16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      bitcoin#17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      bitcoin#18 0x7f499e17b0cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      bitcoin#19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      bitcoin#20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)

  SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in
  MS: 0 ; base unit: 0000000000000000000000000000000000000000
  0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15,
  ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025
  artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d
  Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ==

ACKs for top commit:
  dergoegge:
    ACK fab164f
  brunoerg:
    ACK fab164f

Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2
remyers pushed a commit to remyers/bitcoin that referenced this pull request Jul 26, 2024
refactor: Use util::Split to avoid a harmless unsigned-integer-overflow
fac932b 
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
    bitcoinops#3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
    bitcoinops#4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
    bitcoinops#5 0x7f078ebcbbb3  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
    bitcoinops#6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
    bitcoinops#7 0x7f078e840a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
    bitcoinops#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

Reviewers

@bitschmidty bitschmidty bitschmidty left review comments

+1 more reviewer

@0xB10C 0xB10C 0xB10C left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jnewbery @bitschmidty @0xB10C