Core-Wallets/monacoin
3
0
Fork 0
Code Issues Pull requests Projects Releases 18 Packages Wiki Activity Actions

Update base version to 0.18.x #72

Merged
cryptcoin-junkey merged 29 commits from junkey-dev-0.18 into base-0.18 2021-01-07 01:50:08 +00:00
Conversation 29 Commits 29 Files changed 402 +24686 -10126
cryptcoin-junkey commented 2020-07-11 00:46:24 +00:00 (Migrated from github.com)
Copy link

Just rebased master-0.17 onto litecoin/0.18. It have not been built and tested yet.

Just rebased master-0.17 onto litecoin/0.18. It have not been ~~built and~~ tested yet.
tcejorpniocanom (Migrated from github.com) reviewed 2020-07-11 00:46:24 +00:00
cryptcoin-junkey commented 2020-07-11 01:22:11 +00:00 (Migrated from github.com)
Copy link

[FYI] What I did.

git checkout -b develop-0.18 litecoin/0.18
git rebase --onto HEAD 1b6c48  master-0.17
{loop}
vi {conflicted files}
git add --all
git rebase --continue
{/loop until all conflicts are resolved}

I suppose this way is one of the easiest way.

[FYI] What I did. ``` git checkout -b develop-0.18 litecoin/0.18 git rebase --onto HEAD 1b6c48 master-0.17 {loop} vi {conflicted files} git add --all git rebase --continue {/loop until all conflicts are resolved} ``` I suppose this way is one of the easiest way.
👍 1
cryptcoin-junkey commented 2020-07-11 21:35:27 +00:00 (Migrated from github.com)
Copy link

[FYI] I'll push my commits with --force into this PR branch until it was merged.
Please have in mind if you track this.

[FYI] I'll push my commits with `--force` into this PR branch until it was merged. Please have in mind if you track this.
cryptcoin-junkey commented 2020-07-12 08:49:58 +00:00 (Migrated from github.com)
Copy link

The minimum build (monacoind with '--without-wallet`) was passed.
QT has not been tried yet.

Of course, this doesn't mean "binaries from this tree are stable."

The minimum build (monacoind with '--without-wallet`) was passed. QT has not been tried yet. Of course, this doesn't mean "binaries from this tree are stable."
cryptcoin-junkey commented 2020-07-19 06:09:52 +00:00 (Migrated from github.com)
Copy link

I'm trying full build and found build errors. I'm going to fix them. The workaround is configure --without-wallet for now.

I'm trying full build and found build errors. I'm going to fix them. The workaround is `configure --without-wallet` for now.
wakiyamap commented 2020-07-19 06:31:08 +00:00 (Migrated from github.com)
Copy link

If I find a better place to change, should I write it here?

mv doc/man/litecoin-wallet.1 doc/man/monacoin-wallet.1
mv doc/litecoin-conf.md doc/monacoin-conf.md
If I find a better place to change, should I write it here? ``` mv doc/man/litecoin-wallet.1 doc/man/monacoin-wallet.1 mv doc/litecoin-conf.md doc/monacoin-conf.md ```
❤️ 1
cryptcoin-junkey commented 2020-07-19 07:37:20 +00:00 (Migrated from github.com)
Copy link

@wakiyamap Nice catch. I did it and pushed.

@wakiyamap Nice catch. I did it and pushed.
👍 1
cryptcoin-junkey commented 2020-07-19 08:29:37 +00:00 (Migrated from github.com)
Copy link

I force pushed commits here. I believe monacoin-wallet can be built now.

I force pushed commits here. I believe `monacoin-wallet` can be built now.
wakiyamap (Migrated from github.com) reviewed 2020-07-21 23:24:39 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
wakiyamap (Migrated from github.com) commented 2020-07-21 23:24:39 +00:00
Copy link

If value of nFeePaid is 10512000000000000(MAX_MONEY), it's overflow?
nFeePaid * 1000 =
10,512,000,000,000,000,000
int64_t of max is
9,223,372,036,854,775,807

So......how about adding the following?
assert(nFeePaid<= MAX_MONEY);

Personaly, I'd like to believe that maybe this isn't the case.

If value of `nFeePaid` is 10512000000000000(MAX_MONEY), it's overflow? nFeePaid * 1000 = `10,512,000,000,000,000,000` int64_t of max is `9,223,372,036,854,775,807` So......how about adding the following? `assert(nFeePaid<= MAX_MONEY);` Personaly, I'd like to believe that maybe this isn't the case.
wakiyamap commented 2020-07-21 23:27:57 +00:00 (Migrated from github.com)
Copy link

I could build it. yay!!

I could build it. yay!!
🎉 1
wakiyamap (Migrated from github.com) reviewed 2020-07-22 01:28:00 +00:00
doc/man/monacoin-wallet.1
@ -0,0 +49,4 @@
Create new wallet file
.IP
info
.IP
wakiyamap (Migrated from github.com) commented 2020-07-22 01:28:00 +00:00
Copy link

Monatcoin -> Monacoin

Monatcoin -> Monacoin
wakiyamap commented 2020-07-22 01:30:54 +00:00 (Migrated from github.com)
Copy link

Litecoin -> Monacoin
src/config/bitcoin-config.h

/* Define to the full name of this package. */
#define PACKAGE_NAME "Monacoin Core"

/* Define to the full name and version of this package. */
#define PACKAGE_STRING "Monacoin Core 0.18.1"

src/Makefile

PACKAGE_NAME = Monacoin Core
PACKAGE_STRING = Monacoin Core 0.18.1

image

Litecoin -> Monacoin src/config/bitcoin-config.h ``` /* Define to the full name of this package. */ #define PACKAGE_NAME "Monacoin Core" /* Define to the full name and version of this package. */ #define PACKAGE_STRING "Monacoin Core 0.18.1" ``` src/Makefile ``` PACKAGE_NAME = Monacoin Core PACKAGE_STRING = Monacoin Core 0.18.1 ``` ![image](https://user-images.githubusercontent.com/7189933/88123668-58ce3300-cc06-11ea-95d4-47aa05d4bfce.png)
cryptcoin-junkey (Migrated from github.com) reviewed 2020-07-30 05:52:18 +00:00
doc/man/monacoin-wallet.1
@ -0,0 +49,4 @@
Create new wallet file
.IP
info
.IP
cryptcoin-junkey (Migrated from github.com) commented 2020-07-30 05:52:18 +00:00
Copy link

Nice catch! I fixed it.

Nice catch! I fixed it.
cryptcoin-junkey commented 2020-07-30 05:54:23 +00:00 (Migrated from github.com)
Copy link

@wakiyamap I see. They should be fixed AC_INIT in configure.ac. I did it and pushed here.

@wakiyamap I see. They should be fixed `AC_INIT` in `configure.ac`. I did it and pushed here.
👍 2
yamada-guro-baru (Migrated from github.com) reviewed 2020-08-01 00:32:35 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
yamada-guro-baru (Migrated from github.com) commented 2020-08-01 00:32:34 +00:00
Copy link

x assert(nFeePaid <= MAX_MONEY);
o assert(nFeePaid <= INT64_MAX / 1000);

MAX_MONEY is niet goed genoeg.

x `assert(nFeePaid <= MAX_MONEY);` o `assert(nFeePaid <= INT64_MAX / 1000);` `MAX_MONEY` is niet goed genoeg.
wakiyamap commented 2020-08-04 19:32:43 +00:00 (Migrated from github.com)
Copy link

There is a python module called monacoin_scrypt in the source.
Should I have monacoin_scrypt in pypi?
Ex.. https://pypi.org/project/litecoin_scrypt/

There is a python module called monacoin_scrypt in the source. Should I have monacoin_scrypt in pypi? Ex.. https://pypi.org/project/litecoin_scrypt/
yamada-guro-baru commented 2020-08-05 08:50:20 +00:00 (Migrated from github.com)
Copy link

@wakiyamap
lyra2re2_hash, I thought.......
But, If we're just testing, maybe scrypt is the way to go.

@wakiyamap lyra2re2_hash, I thought....... But, If we're just testing, maybe scrypt is the way to go.
cryptcoin-junkey commented 2020-08-10 00:09:44 +00:00 (Migrated from github.com)
Copy link

@wakiyamap I grepped in the branch.
And I found monacoin_scrypt related code in .travis.yml and `test/functional/test_framework/messages.py'

In .travis.yml, we don't need to care because $MONACOIN_SCRYPT won't set to 1.

IMO, message.py will be useless now even if the PIP module was applied. Totally tests have been broken.

I agree that tests should be refactored for Monacoin. Someday we could do it.
But code in this branch will have similar quality to the released code even if we didn't refactor.

@wakiyamap I grepped in the branch. And I found `monacoin_scrypt` related code in `.travis.yml` and `test/functional/test_framework/messages.py' In `.travis.yml`, we don't need to care because `$MONACOIN_SCRYPT` won't set to `1`. IMO, `message.py` will be useless now even if the PIP module was applied. Totally tests have been broken. I agree that tests should be refactored for Monacoin. Someday we could do it. But code in this branch will have similar quality to the released code even if we didn't refactor.
👍 1
cryptcoin-junkey (Migrated from github.com) reviewed 2020-08-10 00:25:57 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
cryptcoin-junkey (Migrated from github.com) commented 2020-08-10 00:25:57 +00:00
Copy link

Is this a good practice to use asset() here?
I suppose all Monacoin-core nodes will be aborted by assertion failed.
assert() in current code is checks for the runtime (compiler) limitations. Not for argument checks.

My alternative plan is to use int128_t and/or uint128_t in functions. WDYT?

Is this a good practice to use `asset()` here? I suppose all Monacoin-core nodes will be aborted by assertion failed. `assert()` in current code is checks for the runtime (compiler) limitations. Not for argument checks. My alternative plan is to use `int128_t` and/or `uint128_t` in functions. WDYT?
wakiyamap commented 2020-08-10 00:35:13 +00:00 (Migrated from github.com)
Copy link

@cryptcoin-junkey
ok. I agree. I think it's not necessary to all test it ""now"".
This is fine for the base(0.18.x).

But if we're going to release it, please just add maxfeerate.

@cryptcoin-junkey ok. I agree. I think it's not necessary to all test it ""now"". This is fine for the base(0.18.x). But if we're going to release it, please just add `maxfeerate`.
wakiyamap (Migrated from github.com) reviewed 2020-08-10 00:52:20 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
wakiyamap (Migrated from github.com) commented 2020-08-10 00:52:20 +00:00
Copy link

It's good.
I'm not very familiar with C++, but it looks like it says you have to use BOOST.
If you're putting in a new library, so......is it better to use arith_uint256?

It's good. I'm not very familiar with C++, but it looks like it says you have to use `BOOST`. If you're putting in a new library, so......is it better to use `arith_uint256`?
yamada-guro-baru (Migrated from github.com) reviewed 2020-08-13 09:36:38 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
yamada-guro-baru (Migrated from github.com) commented 2020-08-13 09:36:38 +00:00
Copy link

...fm
nFeePaid * 1000 = 10,512,000,000,000,000,000
max of uint64_t = 18,446,744,073,709,551,615
so,

    uint64_t uFeePaid = std::abs(nFeePaid) * 1000;
    if (nSize > 0 && nFeePaid > 0) {
        nSatoshisPerK = CAmount(uFeePaid / nSize);
    } else if (nSize > 0 && nFeePaid < 0) {
        nSatoshisPerK = CAmount(-uFeePaid / nSize);
    } else {
        nSatoshisPerK = 0;
    }

How do you think?

...fm nFeePaid * 1000 = 10,512,000,000,000,000,000 max of uint64_t = 18,446,744,073,709,551,615 so, ``` uint64_t uFeePaid = std::abs(nFeePaid) * 1000; if (nSize > 0 && nFeePaid > 0) { nSatoshisPerK = CAmount(uFeePaid / nSize); } else if (nSize > 0 && nFeePaid < 0) { nSatoshisPerK = CAmount(-uFeePaid / nSize); } else { nSatoshisPerK = 0; } ``` How do you think?
cryptcoin-junkey (Migrated from github.com) reviewed 2020-09-11 01:40:12 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
cryptcoin-junkey (Migrated from github.com) commented 2020-09-11 01:40:12 +00:00
Copy link

How about fixing the type of CAmount ?

In src/amount.h:

  /** Amount in satoshis (Can be negative) */
- typedef int64_t CAmount;
+ typedef int128_t CAmount;

It looks this was discussed in the bitcoin core team.

11:16 < Raystonn> Normally I'd agree. But upon testing a change of CAmount to boost::multiprecision::int128_t, the floating point operations aren't even supported. I could add them, but it could lead to dropped precision or accuracy some place where it matters simply by defining the operators.
11:16 < sipa> there is no need for 128 bit integers
11:16 < Raystonn> Right now no.

They didn't need it. And it may be worth to consider for us.

How about fixing the type of `CAmount` ? In `src/amount.h`: ```diff /** Amount in satoshis (Can be negative) */ - typedef int64_t CAmount; + typedef int128_t CAmount; ``` It looks this was discussed in [the bitcoin core team](http://gnusha.org/bitcoin-core-dev/2019-07-15.log). > 11:16 < Raystonn> Normally I'd agree. But upon testing a change of CAmount to boost::multiprecision::int128_t, the floating point operations aren't even supported. I could add them, but it could lead to dropped precision or accuracy some place where it matters simply by defining the operators. > 11:16 < sipa> there is no need for 128 bit integers > 11:16 < Raystonn> Right now no. They didn't need it. And it may be worth to consider for us.
cryptcoin-junkey (Migrated from github.com) reviewed 2020-09-11 01:52:22 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
cryptcoin-junkey (Migrated from github.com) commented 2020-09-11 01:52:22 +00:00
Copy link

Ooops, s/int128_t/__int128_t/ . AFAIK, __int128_t is GCC extension, supported by Clang also.

Ooops, `s/int128_t/__int128_t/` . AFAIK, `__int128_t` is GCC extension, supported by Clang also.
cryptcoin-junkey (Migrated from github.com) reviewed 2020-09-11 01:55:58 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
cryptcoin-junkey (Migrated from github.com) commented 2020-09-11 01:55:58 +00:00
Copy link

Another plan. I guess this plan is safer.

-         nSatoshisPerK = nFeePaid * 1000 / nSize;
+         nSatoshisPerK = (CAmount)((__int128_t)nFeePaid * 1000 / nSize);
Another plan. I guess this plan is safer. ```diff - nSatoshisPerK = nFeePaid * 1000 / nSize; + nSatoshisPerK = (CAmount)((__int128_t)nFeePaid * 1000 / nSize); ```
wakiyamap (Migrated from github.com) reviewed 2020-09-11 05:50:15 +00:00
src/policy/feerate.cpp
@ -19,1 +18,3 @@
else
if (nSize > 0) {
__int128_t n = (__int128_t)nFeePaid * 1000 / nSize;
assert(n <= INT64_MAX);
wakiyamap (Migrated from github.com) commented 2020-09-11 05:50:15 +00:00
Copy link

@cryptcoin-junkey
I disagree with setting CAmount to int128_t.
The impact is toooooooooo great.

I agree to an another plan.

@cryptcoin-junkey I disagree with setting CAmount to `int128_t`. The impact is toooooooooo great. I agree to an another plan.
wakiyamap commented 2020-12-08 04:55:22 +00:00 (Migrated from github.com)
Copy link

[English]
Why not decide on the criteria to release?
The overflow fix is complete, etc......

My suggestion.
As soon as the overflow fix is complete, run it on your own node for a month or so and release it if it's OK.

[日本語]
リリースする基準を決めませんか？
オーバーフローの修正が完了したら等……

私としてはオーバーフローの修正が完了して、1か月ほど各自でノードを運用して問題なければリリースで良いかなと思います。

[English] Why not decide on the criteria to release? The overflow fix is complete, etc...... My suggestion. As soon as the overflow fix is complete, run it on your own node for a month or so and release it if it's OK. [日本語] リリースする基準を決めませんか？ オーバーフローの修正が完了したら等…… 私としてはオーバーフローの修正が完了して、1か月ほど各自でノードを運用して問題なければリリースで良いかなと思います。
❤️ 2
cryptcoin-junkey commented 2021-01-02 04:00:12 +00:00 (Migrated from github.com)
Copy link

I pushed the fix against #76.

I pushed the fix against #76.
🎉 1
wakiyamap (Migrated from github.com) approved these changes 2021-01-02 04:03:48 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
tcejorpniocanom
wakiyamap
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
help wanted

   
invalid

   
question

   
wontfix
No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Core-Wallets/monacoin!72
No description provided.