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.

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

Fix p2p_ibd_txrelay.py Test & wallet_txn_doublespend.py (4 tests) #193

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

JaredTate
Copy link

@JaredTate JaredTate commented Mar 12, 2024

This fixes the functional p2p_ibd_txrelay.py & wallet_txn_doublespend.py tests. I have been running into a lot of issues with the 20 remaining tests. Speficically around high fees, mempool and psbt issues. In this test, I had to add a bunch of log .info's to see what value for MAX_FEE_FILTER was looking for. No where in the core protocol is MAX_FEE_FILTER even defined, it's specific to this test.

Screenshot 2024-03-12 at 12 36 50 PM

This log.info I added is the line that gave me an answer to what MAX_FEE_FILTER was looking for, but not sure why it comes up with that value, or where the max fee is being set. I just moved decimal all the way to right and test passed. :)

2024-03-12T18:35:15.073000Z TestFramework (INFO): Node 0 peer 0 minfeefilter: 99.36506125

The nearest place in core I can find affecting this test and MAX_FEE_FILTER is here not sure:

digibyte/src/policy/fees.h

Lines 173 to 174 in 3fd7c74

static constexpr double MIN_BUCKET_FEERATE = 1000;
static constexpr double MAX_BUCKET_FEERATE = 1e10;

To test this:

Compile:

  ./autogen.sh
  ./configure
  make -j 8

Run Specific Functional Tests

test/functional/test_runner.py p2p_ibd_txrelay.py wallet_txn_doublespend.py

Screenshot 2024-03-12 at 12 42 06 PM

Perhaps someone else can more clearly explain whats happening here. My brain is a bit fried today.

Edit:
Just pushed fix for wallet_txn_doublespend.py as well so I can switch branches and test GTOs m1 fix.

@ghost
Copy link

ghost commented Mar 12, 2024

When i change
static constexpr double MAX_BUCKET_FEERATE = 1e10;
back to 1e7 ( like Bitcoin and DigiByte 8 rc2 ) i still get the error. so MAX_BUCKET_FEERATE is not the problem.
Screenshot from 2024-03-12 20-04-04

@JaredTate
Copy link
Author

JaredTate commented Mar 12, 2024

This code is a Python test script for DigiByte. It tests the behavior of fee filters during and after the Initial Block Download (IBD) phase.

Here's a breakdown of what the code is doing:

The script defines two constants:

MAX_FEE_FILTER: Set to a high value of 9936506125 / COIN, which represents the maximum fee filter value during IBD.
NORMAL_FEE_FILTER: Set to a lower value of 1000 / COIN, which represents the normal fee filter value after IBD.

COIN is defined here:

COIN = 100000000 # 1 btc in satoshis

So if we take 9936506125 and divide by 100000000 we get, 99.36506125 wich is what my log info said. So that's where the high fee is coming in, which seems right. So this makes sense now.

But where in the actual core protocol is this being set during IBD?

@JaredTate
Copy link
Author

When i change static constexpr double MAX_BUCKET_FEERATE = 1e10; back to 1e7 ( like Bitcoin and DigiByte 8 rc2 ) i still get the error. so MAX_BUCKET_FEERATE is not the problem. Screenshot from 2024-03-12 20-04-04

I just did similar test, and found the same conclusion.

@ghost
Copy link

ghost commented Mar 12, 2024

This code is a Python test script for DigiByte. It tests the behavior of fee filters during and after the Initial Block Download (IBD) phase.

Here's a breakdown of what the code is doing:

The script defines two constants:

MAX_FEE_FILTER: Set to a high value of 9936506125 / COIN, which represents the maximum fee filter value during IBD. NORMAL_FEE_FILTER: Set to a lower value of 1000 / COIN, which represents the normal fee filter value after IBD.

COIN is defined here:

COIN = 100000000 # 1 btc in satoshis

So if we take 9936506125 and divide by 100000000 we get, 99.36506125 wich is what my log info said. So that's where the high fee is coming in, which seems right. So this makes sense now.

But where in the actual core protocol is this being set during IBD?

constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN * 100};

@JaredTate
Copy link
Author

JaredTate commented Mar 12, 2024

It appears the purpose of this test is to ensure that during IBD (initial block download), the nodes set their fee filter to a high value MAX_FEE_FILTER to prioritize the download of blocks and avoid processing low-fee transactions. Once IBD is complete, the nodes should reset their fee filter to the normal value NORMAL_FEE_FILTER to start processing transactions with lower fees.

It looks like its may be here:

//! -maxtxfee default
constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN * 100};
//! Discourage users to set fees higher than this amount (in satoshis) per kB
constexpr CAmount HIGH_TX_FEE_PER_KB{COIN};
//! -maxtxfee will warn if called with a higher fee than this amount (in satoshis)
constexpr CAmount HIGH_MAX_TX_FEE{1000 * HIGH_TX_FEE_PER_KB};

Coin in core protocol defined here:

static const CAmount COIN = 100000000;

So if we do 100000000 x 100 we get: 100.00000000 DGB max fee.

Is that math correct? That should be about the right fee range. So test working as expected and core protocol settings good. I am good with this test now. Cheers

@ghost
Copy link

ghost commented Mar 12, 2024

Screenshot from 2024-03-12 20-58-03

ACK Tested. nice work @JaredTate !!

ghost
ghost previously approved these changes Mar 12, 2024
@ghost ghost assigned ycagel, gto90 and j50ng and unassigned ycagel, gto90 and j50ng Mar 12, 2024
@JaredTate JaredTate changed the title Fix p2p_ibd_txrelay.py Test Fix p2p_ibd_txrelay.py Test & wallet_txn_doublespend.py (4 tests) Mar 12, 2024
@JaredTate
Copy link
Author

JaredTate commented Mar 12, 2024

I just pushed 3 more test fixes for wallet_txn_doublespend.py so I can switch branches and test GTO's m1 fix. Sorry @Jongjan88
Screenshot 2024-03-12 at 4 09 14 PM

I wont push anymore to this PR.

Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

utACK

@ghost ghost self-requested a review March 12, 2024 22:39
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

utACK Nice work @JaredTate !

Copy link
Member

@ycagel ycagel left a comment

Choose a reason for hiding this comment

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

cACK. Good work @JaredTate.

@ycagel ycagel merged commit d4089cc into DigiByte-Core:develop Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants