release 0.14.2 #755
Merged
release 0.14.2 #755
Conversation
This file contains hidden or 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
warnings are generated e.g.
from ./chainparams.h:9,
from test/blockmanager_tests.cpp:5:
./protocol.h:316:26: warning: redundant redeclaration of ‘std::vector<std::__cxx11::basic_string<char> > serviceFlagsToStr(uint64_t)’ in same scope [-Wredundant-decls]
316 | std::vector<std::string> serviceFlagsToStr(uint64_t flags);
| ^~~~~~~~~~~~~~~~~
./protocol.h:309:26: note: previous declaration of ‘std::vector<std::__cxx11::basic_string<char> > serviceFlagsToStr(uint64_t)’
309 | std::vector<std::string> serviceFlagsToStr(uint64_t flags);
| ^~~~~~~~~~~~~~~~~
warnings are generated e.g.
from ./chainparams.h:9,
from test/blockmanager_tests.cpp:5:
./protocol.h:316:26: warning: redundant redeclaration of ‘std::vector<std::__cxx11::basic_string<char> > serviceFlagsToStr(uint64_t)’ in same scope [-Wredundant-decls]
316 | std::vector<std::string> serviceFlagsToStr(uint64_t flags);
| ^~~~~~~~~~~~~~~~~
./protocol.h:309:26: note: previous declaration of ‘std::vector<std::__cxx11::basic_string<char> > serviceFlagsToStr(uint64_t)’
309 | std::vector<std::string> serviceFlagsToStr(uint64_t flags);
| ^~~~~~~~~~~~~~~~~
MatthewLM
requested changes
May 3, 2024
There was a problem hiding this comment.
To summarise my suggested improvement:
- Add up-to 8 inputs assuming only 1 output will be added.
- In the loop to create the split outputs
- Re-run the split logic after determining the fee with the new required fee. Remove the complex
nDeductlogic. - If the tx is over 1KB, start by removing added inputs one at a time and rerunning the loop.
- Re-run the split logic after determining the fee with the new required fee. Remove the complex
- If there is only the original input left (or allowing up to 4?) and the tx still exceeds the 1KB limit, then remove as many outputs as required to bring the tx below 1KB and then split the output amount evenly across those remaining outputs.
|
@backpacker69 I've not tested this, but it should be clear what I'm suggesting from this code: CAmount nMinFee = 0;
int maxOutputs = 100;
CAmount nMinFeeBase = (IsProtocolV07(txNew.nTime) ? MIN_TX_FEE : MIN_TX_FEE_PREV7);
while(true) {
// Clear outputs
txNew.vout.erase(txNew.vout.begin() + 1+bMinterKey, txNew.vout.end());
// Assume success
bool outputsOk = true;
// split and set amounts based on rfc28
if (pwallet->m_split_coins) {
CAmount current = nCredit - nMinFee;
double ratio = current / nTarget;
// Obtain the optimal number of outputs and clamp it to maxOutputs to ensure the fee is not exceeded
int desiredOutputs = min(
int(std::floor((std::sqrt(4 * std::pow(ratio, 2) + 1) + 1) / 2)),
maxOutputs,
);
if (bDebug)
LogPrintf("rfc28: security level is %f, target amount %f, desired outputs %d from %f ppc\n",
securityLevel, double(nTarget)/COIN, desiredOutputs, double(current)/COIN);
CAmount outValue = current / desiredOutputs;
CAmount remainder = current - outValue*desiredOutputs;
for (int i=0; i<desiredOutputs; i++) {
// Add remainder to first output
txNew.vout.push_back(CTxOut(outValue + (i == 0 ? remainder : 0), scriptPubKeyOut));
// make sure transaction below 1kb
// Why 950?
if (::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION) > 950) {
// Remove output that goes over max size and set maxOutputs so that the logic can be re-run
txNew.vout.pop_back();
outputsOk = false;
maxOutputs = i;
break;
}
}
} else {
txNew.vout.push_back(CTxOut(nCredit - nMinFee, scriptPubKeyOut));
}
// Sign
// SNIPPED CODE FOR BREVITY. ADD THIS BACK
// Limit size
unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION);
if (nBytes >= 1000000/5)
return error("CreateCoinStake : exceeded coinstake size limit");
// Check enough fee is paid
CAmount nNeededFee = GetMinFee(CTransaction(txNew), txNew.nTime) - nMinFeeBase;
// Not enough fee paid or max outputs have changed requiring a new split and fee, so run again
if (nMinFee < nNeededFee || !outputsOk) {
nMinFee = nNeededFee;
continue; // try signing again
} else {
if (bDebug)
LogPrintf("CreateCoinStake : fee for coinstake %s\n", FormatMoney(nMinFee).c_str());
break;
}
} |
MatthewLM
reviewed
May 6, 2024
There was a problem hiding this comment.
Looks good, though please check the fee-free coinstake size.
I've not tested it however. Certain edge cases should be tested before release. Automated functional tests would be ideal, but without that manual testing of the different branching behaviour and constraints should be done.
I don't expect there should be any problems but I would release this as an RC and ask people to test out on testnet to see if any problems arise before final release.
MatthewLM
reviewed
May 6, 2024
MatthewLM
reviewed
May 6, 2024
|
i have manually tested the edge cases including minter key and added extra safety handling of situation when size of inputs exceeds 1000 bytes. |
MatthewLM
reviewed
May 7, 2024
MatthewLM
approved these changes
May 7, 2024
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.
No description provided.