Skip to content
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

Addrman check failure info.nLastSuccess is 0 even though info.fInTried #22503

Closed
maflcko opened this issue Jul 20, 2021 · 0 comments · Fixed by #22734
Closed

Addrman check failure info.nLastSuccess is 0 even though info.fInTried #22503

maflcko opened this issue Jul 20, 2021 · 0 comments · Fixed by #22734
Labels
Bug

Comments

@maflcko
Copy link
Member

maflcko commented Jul 20, 2021

Using #22493 + #22500 gives the failure mentioned in the subject line

crash-2e85f5a8d3623503ed9c668d02dc65c5bb573df1.log
@maflcko maflcko added the Bug label Jul 20, 2021
@maflcko maflcko changed the title Addrman check failure info.nLastSuccess is false even though info.fInTried Addrman check failure info.nLastSuccess is 0 even though info.fInTried Jul 23, 2021
@mzumsande mzumsande mentioned this issue Aug 13, 2021
Closed
@maflcko maflcko mentioned this issue Aug 18, 2021
Merged
@maflcko maflcko mentioned this issue Sep 10, 2021
Closed
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Sep 21, 2021
Merge bitcoin#22734: addrman: Avoid crash on corrupt data, Force Chec…
43bc877 
…k after deserialize

fa3669f fuzz: Move all addrman fuzz targets to one file (MarcoFalke)
fa7a883 addrman: Replace assert with throw on corrupt data (MarcoFalke)
fa29897 Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke)
fae5c63 move-only: Move CAddrMan::Check to cpp file (MarcoFalke)

Pull request description:

  Assert should only be used for program internal logic errors, not to sanitize external user input.

  The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70, thus won't need a backport.

  Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.

  For example, if `nLastSuccess` is negative, it would  later result in integer overflows. Thus, this patch fixes bitcoin#22931.

  Also,
  Fixes bitcoin#22503
  Fixes bitcoin#22504
  Fixes bitcoin#22519

  Closes bitcoin#22498

  Steps to test:

  ```
  mkdir -p /tmp/test_235/regtest/
  echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
  ./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'
  ```

  Output before:
  ```
  2021-09-10T11:28:37Z init message: Loading P2P addresses…
  2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
  bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.

  (program crashes)
  ```

  Output after:
  ```
  2021-09-10T11:26:00Z init message: Loading P2P addresses…
  2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.

  (program exits)
  ```

ACKs for top commit:
  naumenkogs:
    ACK fa3669f
  jnewbery:
    Code review ACK fa3669f
  vasild:
    ACK fa3669f

Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
1 participant
@maflcko