test: fix dbcrash and fee_estimation extended tests #1298
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Force pushed to reduce the number of iterations in the dbcrash test, such that it still passes but in a "reasonable" amount of time (about 6 hours in my testing). I have tested these 2 tests manually and they are now passing. |
|
@delta1 am I running the correct test? |
|
@jamesdorfman yes you ran the right test. It seems I've reduced the values too much for it to fail (correctly) every time. Bumped the number of iterations from 3 to 4, please test again. It will take closer to 8 hours now. |
|
latest run: approx 6 hours |
|
Hmm... seems to still be happening though: |
|
strange, worked for me again. i'm going to bump up the iteration and run it again. |
|
5 iterations and |
|
@jamesdorfman had a similar failure. bumped the iterations to 6 and will run a number of tests |
|
@delta1: It looks as though you're trying to tune a probabilistic test. You're never going to find a number that gives the desired result on every machine from the slowest single-core SoC to the fastest NUMA supercomputer. Instead, the test case should be fixed so that it is deterministic. If that is not possible, then the test case should be discarded. |
|
I suspect there’s a threshold that works reliably like the Bitcoin test. |
JBetz
added a commit
to SequentiaSEQ/SEQ-Core-Elements
that referenced
this pull request
May 2, 2024
JBetz
added a commit
to SequentiaSEQ/SEQ-Core-Elements
that referenced
this pull request
May 3, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
JBetz
added a commit
to SequentiaSEQ/SEQ-Core-Elements
that referenced
this pull request
May 3, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
JBetz
added a commit
to SequentiaSEQ/SEQ-Core-Elements
that referenced
this pull request
May 3, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
Mixa84
pushed a commit
to SequentiaSEQ/SEQ-Core-Elements
that referenced
this pull request
May 6, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems these tests have never been run for Elements, since the extended tests are not run in the CI process.
This PR fixes both of them, however there may be a further issue with
feature_dbcrash.pyrunning for an extremely long time which we are still in the process of testing.#1297
#1296
cc @whitslack