test: Wait for local services to update in feature_assumeutxo #30880

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2024-09-au-services-test changing 1 files +3 −3
  1. fjahr commented at 2:18 pm on September 12, 2024: contributor

    Closes #30878

    It seems like there is a race where the block is stored locally and getblock does not error anymore, but ActivateBestChain has not finished yet, so the local services are not updated yet either. Fix this by waiting for the local services to update.

    Can be reproduced locally by adding the sleep here:

    0──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
    1src/validation.cpp:3567: bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< 
    2──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
    3        }
    4
    5        if (WITH_LOCK(::cs_main, return m_disabled)) {
    6            std::this_thread::sleep_for(std::chrono::seconds(10));
    7            // Background chainstate has reached the snapshot base block, so exit.
    8
    9            // Restart indexes to resume indexing for all blocks unique to the snapshot
    
  2. DrahtBot commented at 2:18 pm on September 12, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, pablomartin4btc, furszy, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Sep 12, 2024
  4. pablomartin4btc commented at 2:25 pm on September 12, 2024: member

    ACK d823f59c91cb00fe763b0c036454440f1948bf48

    Perhaps also at this line L701 too:

    assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])

    (It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)

  5. fjahr commented at 2:26 pm on September 12, 2024: contributor

    Perhaps also at this line L701 too:

    assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])

    (It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)

    Yeah, I just saw that after I succeeded to reproduce it locally, so don’t merge yet please. Checking if there are more changes needed.

  6. fanquake added the label Needs backport (28.x) on Sep 12, 2024
  7. test: Wait for local services to update in feature_assumeutxo 19f4a7c95a
  8. fjahr force-pushed on Sep 12, 2024
  9. fjahr commented at 2:37 pm on September 12, 2024: contributor
    Ok, feature_assumeutxo.py should be good now with this, ready for review.
  10. maflcko commented at 2:46 pm on September 12, 2024: member

    review-only ACK 19f4a7c95a99162122068d4badffeea240967a65

    I did not test the reproducer.

  11. DrahtBot requested review from pablomartin4btc on Sep 12, 2024
  12. pablomartin4btc approved
  13. pablomartin4btc commented at 2:53 pm on September 12, 2024: member

    tACK 19f4a7c95a99162122068d4badffeea240967a65

    I’ve managed to reproduce it locally as per instructions in the description, adding a wait for 20 secs and with this PR built there were no errors.

  14. furszy commented at 3:28 pm on September 12, 2024: member

    Code review ACK 19f4a7c.

    There is another potential race at line 621, though it’s different one. This one wouldn’t cause a failure if it occurs, as the node already has the prune node service flag set. The check there verifies that services remain unchanged after the background sync is completed for prune nodes. The worst-case scenario would be if the services are actually changed and we don’t detect it. However, this isn’t an issue because the test would fail intermittently.

  15. achow101 commented at 6:37 pm on September 12, 2024: member
    ACK 19f4a7c95a99162122068d4badffeea240967a65
  16. achow101 merged this on Sep 12, 2024
  17. achow101 closed this on Sep 12, 2024

  18. achow101 referenced this in commit d39262e5d4 on Sep 12, 2024
  19. achow101 commented at 7:03 pm on September 12, 2024: member
    Backported in #30827
  20. fanquake removed the label Needs backport (28.x) on Sep 12, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me