addrman_tests

[ghost]

src/test/addrman_tests.cpp
//after changing ports back from 12024 to 8333 addrman_tests pass successful.

On the screenshot you can see it pass addrman_tests.cpp

[ghost]

v7.17.3

digibyte/src/policy/feerate.cpp

Line 13 in 258afac

CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)

[JaredTate]

Thanks for doing this! A couple of quick questions, other than the fact it was using size_t in 7.17.3 was there any other reason to change this? I think for cross-platform consistency we should use uint32_t.

Chat GPT summary is helpful to understand why:

In C++, uint32_t and size_t are both fundamental types used for different purposes:

uint32_t: This is an unsigned 32-bit integer type defined in the header. It's explicitly sized to be 32 bits wide, regardless of the platform. It is useful when you need a specific width integer, for example, when dealing with data formats or protocols that require 32-bit unsigned integers.

size_t: This is an unsigned integer type defined in the header. It's typically used to represent sizes and indices of objects in memory. Its size may vary depending on the platform; however, it's guaranteed to be large enough to represent the size of the largest object supported by the implementation. This means it's usually the most appropriate type to use when dealing with sizes of arrays, memory allocations, or indices within arrays.

In summary:

uint32_t is explicitly 32 bits wide and is used when you need a fixed-width unsigned integer.
size_t is platform-dependent and is typically used for representing sizes and indices within memory.

Please let me know if I am missing something else with this, but I think BTC devs made the change for cross platform consistency.

[ghost]

Thanks for doing this! A couple of quick questions, other than the fact it was using size_t in 7.17.3 was there any other reason to change this? I think for cross-platform consistency we should use uint32_t.

I saw a problem in test/amount_tests.cpp. The size of DigiByte MAX_MONEY was 21e17. Bitcoin MAX_MONEY is 21e15.
It only worked when i did.

size_t MAX_MONEY2;
MAX_MONEY2 = MAX_MONEY / 1000;
CFeeRate(MAX_MONEY2, std::numeric_limits<uint32_t>::max()).GetFeePerK(); 

digibyte/src/test/amount_tests.cpp

Line 88 in dd95b35

CFeeRate(MAX_MONEY, std::numeric_limits<uint32_t>::max()).GetFeePerK();

Then i saw in policy/feerate.h and policy/feerate.cpp uint32_t. And changed it just to be sure. saw the same at Litecoin and DigiByte 7.

Here num_bytes is converted immediately to int64_t

digibyte/src/policy/feerate.cpp

Lines 12 to 14 in dd95b35

	CFeeRate::CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes)
	{
	const int64_t nSize{num_bytes};

But of course i have changed it back to uint32_t. uint32_t is better. Thank you!!

[ghost]

in src/test/amount_tests.cpp i also change int64_t to uint32_t.

[gto90]

I left some comments for @JaredTate and @Jongjan88 to review.

[gto90]

@Jongjan88 and @JaredTate, I am concerned about this change. Normally, I would not expect changes to our test code but rather the underlying code being tested for the respective tests to pass.

The proposed change, however, alters the test to use a boolean expression result (either 0 or 1) as the first argument to CFeeRate. This means the test no longer checks the behavior of CFeeRate with large numerical values but rather with the smallest possible non-negative values (0 or 1).

This shift in testing could potentially miss issues or behaviors that only manifest with larger values. Changing a test is unusual unless the underlying code or the specifications have changed.

@Jongjan88, can you provide some more detail on this change?

[ghost]

I think you are right. We should not change the test if possible.

When i do this. The code is working.

size_t = MAX_MONEY2
MAX_MONEY2 = MAX_MONEY / 1000;
CFeeRate(MAX_MONEY2, std::numeric_limits<uint32_t>::max()).GetFeePerK();

Update
I remove this line CFeeRate(MAX_MONEY <= uint32_t(std::numeric_limits<uint32_t>::max())).GetFeePerK();

[ycagel]

@Jongjan88 Are these all the correct changes and should other devs be reviewing this?

[ghost]

@Jongjan88 Are these all the correct changes and should other devs be reviewing this?

Yes.

[gto90]

cACK.

This looks good now @Jongjan88 , but I do have a question about placing the assertions in feerate.cpp. While this does make this code more guarded, I wonder what the performance impact will be? I will have to defer to @JaredTate . Conceptually, this PR looks ok though.

[ycagel]

cACK. I agree with @gto90 that we need to get a further review from @JaredTate.

[JaredTate]

Ok, I had reviewed and compiled it yesterday but now there are only 2 changes. Is that correct? Others were reverted?

[gto90]

Ok, I had reviewed and compiled it yesterday but now there are only 2 changes. Is that correct? Others were reverted?

That is correct @JaredTate ! I believe they were reverted because they were changes to the test code which obfuscated underlying issues we are actually having in the code.

[gto90]

@Jongjan88 I see that we use 12024 still in a couple of files, should they also be changed?

• nodes_main.txt
• windows-setup.md
• chainparams.cpp
• netbase.h
• net.cpp
• net_tests.cpp
• netbase_tests.cpp

[gto90]

@Jongjan88 I see that we use 12024 still in a couple of files, should they also be changed?

• nodes_main.txt
• windows-setup.md
• chainparams.cpp
• netbase.h
• net.cpp
• net_tests.cpp
• netbase_tests.cpp

@Jongjan88 , we do not necessarily have to change them in this PR to be clear. But we should make sure that our ports are consistent across the code base.

[ghost]

@Jongjan88 I see that we use 12024 still in a couple of files, should they also be changed?

• nodes_main.txt
• windows-setup.md
• chainparams.cpp
• netbase.h
• net.cpp
• net_tests.cpp
• netbase_tests.cpp

@Jongjan88 , we do not necessarily have to change them in this PR to be clear. But we should make sure that our ports are consistent across the code base.

@gto90

I saw them. I think some need to be changed. chainparams.cpp not ofc.
But haven't come across a test yet that failed due to the port. except addrman_tests
In the future it would be nice that the test also use 12024. But for now i don't think it is a problem to let them on port 8333.
Like before PR #166

#178 (comment)
Need some help on test 1 amount_tests.cpp(88) @JaredTate

@gto90
Haha. My next PRs are definitely going to be better. last 2 PRs were a disaster! Sorry for this.

[gto90]

utACK

Thank you @Jongjan88

[JaredTate]

No worries on this, so now it's just port changes correct? I am trying to free up a couple of full days to really dive in on this. I have had other commitments to finish over this past week. I should free up the next few days.

[ghost]

No worries on this, so now it's just port changes correct? I am trying to free up a couple of full days to really dive in on this. I have had other commitments to finish over this past week. I should free up the next few days.

Nice. Yes after this port change in addrman_tests. addrman_tests will not give 3 errors. And pass successful.

[JaredTate]

ACK. Make sense, thanks for doing this. It compiles & runs good. On to the next!

[ycagel]

cACK. Thanks for your efforts folks!