Transactions submitted by sendrawtransaction rpccall should be always allowed despite this new -relaypriority switch. #6240

wangchun commented at 11:08 AM on June 5, 2015: none

Transactions submitted by sendrawtransaction rpccall should be always allowed despite this new -relaypriority switch.

Allow duplicate txout address in createrawtransaction rpccall.

Added microsecond for logtimestamps.

Tab indent fix.

Transactions submitted by sendrawtransaction rpccall should be always allowed despite this new -relaypriority switch. f653b8bd2d

Allow duplicate txout address in createrawtransaction rpccall 37051ec184

Added microsecond for logtimestamps. 3c3a310bed

Added missing header file. e0a6172107

laanwj added the label RPC on Jun 5, 2015

in src/netbase.cpp:None in e0a6172107

 787 | @@ -788,15 +788,15 @@ bool CNetAddr::IsTor() const
 788 |  bool CNetAddr::IsLocal() const
 789 |  {
 790 |      // IPv4 loopback
 791 | -   if (IsIPv4() && (GetByte(3) == 127 || GetByte(3) == 0))
 792 | -       return true;
 793 | +    if (IsIPv4() && (GetByte(3) == 127 || GetByte(3) == 0))
 794 | +        return true;

jonasschnelli commented at 6:55 PM on June 5, 2015:

Why did you change this lines?

wangchun commented at 3:39 AM on June 6, 2015:

These lines do not follow the code style by only having three space characters for a tab indent.

jonasschnelli commented at 7:51 AM on June 6, 2015:

Okay. I see. You are right that the formatting is wrong there. But the diff you create can be hard to read for others. At least it would be good to separate the style changes from the actual code changes. Also i wouldn't care to much about existing inconsistency in code formatting because we could run one clang-format over everything (see #5387).

in src/netbase.cpp:None in e0a6172107

 801 | +    static const unsigned char pchLocal[16] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1};
 802 | +    if (memcmp(ip, pchLocal, 16) == 0)
 803 | +        return true;
 804 |  
 805 | -   return false;
 806 | +    return false;

jonasschnelli commented at 6:55 PM on June 5, 2015:

Why did you change netbase.cpp at all?

in src/rpcmining.cpp:None in e0a6172107

  85 | @@ -86,7 +86,7 @@ UniValue getnetworkhashps(const UniValue& params, bool fHelp)
  86 |              "\nExamples:\n"
  87 |              + HelpExampleCli("getnetworkhashps", "")
  88 |              + HelpExampleRpc("getnetworkhashps", "")
  89 | -       );
  90 | +        );

jonasschnelli commented at 6:55 PM on June 5, 2015:

Same for this file.

jonasschnelli commented at 6:59 PM on June 5, 2015: contributor

I think only your change in main.cpp line 1003 is relevant. You should probably remove all other changes and force-push again.

jonasschnelli commented at 7:08 PM on June 5, 2015: contributor

I think the concept of allowing local wallet send* commands to always end up in the memory pool is not a undeliberated idea. It didn't thought it through completely but somehow i agree that local wallet send commands should always end up in the local memory pool. Sorry for that, but the code is ugly and unusable. NACK for the code but idea seems usefull

in src/main.cpp:None in e0a6172107

 999 | @@ -1000,7 +1000,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
1000 |                               REJECT_INSUFFICIENTFEE, "insufficient fee");
1001 |  
1002 |          // Require that free transactions have sufficient priority to be mined in the next block.
1003 | -        if (GetBoolArg("-relaypriority", true) && nFees < ::minRelayTxFee.GetFee(nSize) && !AllowFree(view.GetPriority(tx, chainActive.Height() + 1))) {
1004 | +        if (fLimitFree && GetBoolArg("-relaypriority", true) && nFees < ::minRelayTxFee.GetFee(nSize) && !AllowFree(view.GetPriority(tx, chainActive.Height() + 1))) {

jonasschnelli commented at 7:09 PM on June 5, 2015:

I think you misuse fLimitFree here.

in src/util.cpp:None in e0a6172107

 250 | +            std::locale loc(std::locale::classic(), new boost::posix_time::time_facet("%Y-%m-%dT%H:%M:%sZ"));
 251 | +            std::stringstream ss;
 252 | +            ss.imbue(loc);
 253 | +            ss << boost::posix_time::microsec_clock::universal_time();
 254 | +            ret += fprintf(fileout, "[%s] ", ss.str().c_str());
 255 | +        }

jonasschnelli commented at 7:52 AM on June 6, 2015:

What is the reason for this? Is this required to achieve your PRs goal or is this another feature that is unrelated to this PR?

laanwj commented at 7:54 AM on June 6, 2015: member

To make it easier to discuss specific changes. It's better to submit unrelated changes separately:

Transactions submitted by sendrawtransaction rpccall should be always allowed despite this new -relaypriority switch: this may be slightly controversial - we've always have the implicit policy 'don't send transactions that you wouldn't relay yourself'
Allow duplicate txout address in createrawtransaction rpccall: fair enough
Added microsecond for logtimestamps: this is a feature unrelated to your RPC changes
Tab indent fix: whitespace 'improvents', if you do them at all, should go to @theuni's trivial-next branch: https://github.com/theuni/bitcoin

luke-jr commented at 11:51 PM on June 14, 2015: member

I don't see a reason to allow address reuse in createrawtransaction.

sipa commented at 1:51 PM on July 9, 2015: member

Closing until @laanwj's comments are addressed: this PR tries to do many things at once, making it hard to review.

sipa closed this on Jul 9, 2015

MarcoFalke locked this on Sep 8, 2021