Un-color the scammers warning when using testnet.

katesalazar commented at 1:44 pm on July 28, 2021: contributor

If scammers are active, telling users to type commands in the console, stealing their testnet wallet contents, maybe someone can send them some more testnet coins.

Split string. 0885b5068e

Un-color the scammers warning when using testnet. 7d67af2dfc

in src/qt/rpcconsole.cpp:872 in 7d67af2dfc outdated

873                  "<b>help</b>",
874-                 "<b>help-console</b>",
875-                 "<span class=\"secwarning\">",
876+                 "<b>help-console</b>") +
877+        tr(
878+           "%7WARNING: Scammers have been active, telling users to type"

laanwj commented at 2:31 pm on July 28, 2021:

I think this should be %1 not %7 (and %2 instead of %8 below) now that it is split to a separate message?

Peer review.

DISCLAIMER: ATTOW THIS ISN'T LEAK CHECKED.

laanwj wrote:

    I think this should be %1 not %7 (and %2 instead of %8 below) now
    that it is split to a separate message?

Yes, I forgot to change that.

laanwj also wrote:

    Wonder if it would make sense to move the secwarning style to
    NetworkStyle instead of hardcoding a comparison to testnet here.
    The idea of that class is to make network-specific style
    data-driven.

IDK / it could be / no strong opinion.

0c85f16b8b

in src/qt/rpcconsole.cpp:875 in 7d67af2dfc outdated

876+                 "<b>help-console</b>") +
877+        tr(
878+           "%7WARNING: Scammers have been active, telling users to type"
879+           " commands here, stealing their wallet contents. Do not use this console"
880+           " without fully understanding the ramifications of a command.%8")
881+            .arg(gArgs.GetChainName() == CBaseChainParams::TESTNET ? "<span>" : "<span class=\"secwarning\">",

laanwj commented at 2:33 pm on July 28, 2021:

Wonder if it would make sense to the secwarning style to NetworkStyle instead of hardcoding a comparison to testnet here. The idea of that class is to make network-specific style data-driven.

katesalazar commented at 9:22 pm on August 10, 2021:

I’m not convinced that making the secwarning style dependent on which network is much better than replacing the styled span for a vanilla span (or even deleting the span and nested text, completely) depending on which network (and then maybe removing the secwarning style in those cases because it’s not really used by any other string).

However, the hardcoded comparison was horrible and embarrassing so it had to go away right away, deferring doubts on the “what exactly” to some later moment after having fixed the “how”.

katesalazar commented at 5:35 pm on July 28, 2021: contributor

Leak bugs, set draft.

katesalazar marked this as a draft on Jul 28, 2021

katesalazar commented at 5:40 pm on July 28, 2021: contributor

I’m developing this using a powerful Apple machine, but I’m not familiar with leak debugging on macOS.

I can use Memcheck, but I don’t have any decent hardware running Linux.

Would be too much to ask for, to abuse CI as a leak debugger?

WIP 268c302bcf

luke-jr changes_requested

luke-jr commented at 8:58 pm on July 28, 2021: member

Concept NACK. Testnet is for testing. It should be as close to mainnet use as possible. There’s no reason to treat it specially here. Even worse, the code for doing it is excessively complex.

katesalazar commented at 2:42 am on July 29, 2021: contributor

I can’t prove it introduces leak bugs, unset draft.

katesalazar marked this as ready for review on Jul 29, 2021

katesalazar commented at 3:10 pm on July 29, 2021: contributor

Before merging, if that were the case, 268c302bcfcd7898358a695984c2f028d3baab74 should be collapsed to 0c85f16b8b0bdf7387b1fd2792448863efdaf97f and 0c85f16b8b0bdf7387b1fd2792448863efdaf97f should be reworded. Set draft.

katesalazar marked this as a draft on Jul 29, 2021

katesalazar commented at 9:42 pm on July 30, 2021: contributor

luke-jr wrote:

Concept NACK. Testnet is for testing. It should be as close to mainnet use as possible. There’s no reason to treat it specially here. Even worse, the code for doing it is excessively complex.

You make an excellent point in general; but specifically about complexity, I can’t speak for every desktop environment, but today in my end I’m seeing that red[0] and specially #006060[1] are much better suited for contrast in Plasma Breeze than in Plasma Breeze Dark.

Of course maybe Plasma Breeze Dark is broken, IDK, but if so, maybe it even will be dismantled before it gets ever fixed, but the point here is… because of recent tendency in user interfaces and specially increasing demand (or offer?) of dark/light preferences support, you can stop it today, but it’s possible that you might be inevitably going to have a bit of complexity increase here in the end, in order to support more precise color schemes.

o/

[0]: https://github.com/bitcoin/bitcoin/blob/da1c0c64fd094880712d1c4167ad9eb3bb6ffcc6/src/qt/rpcconsole.cpp#L849

[1]: https://github.com/bitcoin/bitcoin/blob/da1c0c64fd094880712d1c4167ad9eb3bb6ffcc6/src/qt/rpcconsole.cpp#L848

achow101 commented at 10:34 pm on August 4, 2021: member

NACK

This change is not well motivated. I see no reason to add extra complexity for a color change that has no real effect on users.

If the problem is that the colors don’t work in certain environments, then a more general change to the color scheme (and perhaps making it more flexible) would be more appropriate than a targeted change to this one specific line of text.

luke-jr commented at 11:27 pm on August 4, 2021: member

There’s definitely work to be done on supporting user themes/palettes, but this isn’t the right approach. (Might I suggest reviving and troubleshooting https://github.com/bitcoin/bitcoin/pull/8889 as a good place to start? ;)

in src/qt/rpcconsole.cpp:858 in 268c302bcf

865         );
866 
867     static const QString welcome_message =
868-        /*: RPC console welcome message.
869-            Placeholders %7 and %8 are style tags for the warning content, and
870-            they are not space separated from the rest of the text intentionally. */

hebasto commented at 11:35 am on August 5, 2021:

Why is this removed?

katesalazar commented at 2:58 pm on August 5, 2021:

Hi. The RPC console welcome message line is removed because this line: https://github.com/bitcoin-core/gui/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/qt/rpcconsole.cpp#L856 is useless comment, as ‘welcome message’ is already the name of the string, and ‘RPC console’ is already the name of the class.

The other two lines: https://github.com/bitcoin-core/gui/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/qt/rpcconsole.cpp#L857-L858 are useful comment, and it’s moved to somewhere else by my rev https://github.com/bitcoin-core/gui/commit/0c85f16b8b0bdf7387b1fd2792448863efdaf97f. I’m sorry it’s my fault that is confusing, because is a change that should had happened before, in my rev https://github.com/bitcoin-core/gui/pull/387/commits/0885b5068edd1753876d66fba272527add22bc3c, not where it is now. If this were ever to be merged, I would move that change, that comment movement, to the rev were it belongs.

I hope I have explained myself clearly enough. ^^

Cheers,

hebasto commented at 3:00 pm on August 5, 2021:

is useless comment, as ‘welcome message’ is already the name of the string, and ‘RPC console’ is already the name of the class.

This is a translator comment dedicated to provide a context to translators on Transifex.com.

katesalazar commented at 3:18 pm on August 5, 2021:

is useless comment, as ‘welcome message’ is already the name of the string, and ‘RPC console’ is already the name of the class.

This is a translator comment dedicated to provide a context to translators on Transifex.com.

Aha, thanks for showing me. I had noticed the colon when coding, it felt quite weird and I thought it should be there for some reason, just I didn’t know it would fire back at me this soon.

Cheers,

the mission becomes to make the tools hapy

                    laanwj, 2021-08-02T11:16:25, #bitcoin-core-dev

hebasto commented at 11:37 am on August 5, 2021: member

NACK for the reasons mentioned above by @luke-jr and @achow101.

hebasto commented at 12:10 pm on August 7, 2021: member

This PR looks pretty controversial: 1 Concept NACK, 2 NACKs.

Going to close it.

hebasto added the label Design on Aug 7, 2021

jarolrod commented at 3:18 am on August 10, 2021: member

I am concept NACK on this, not worth added complexity or effort

hebasto commented at 8:31 am on August 10, 2021: member

Closing for now.

hebasto closed this on Aug 10, 2021

laanwj commented at 9:13 am on August 10, 2021: member

I like the general idea of making the network style contribute part of the style sheet as way of accomplishing this. I think this is much better than the initial implementation, which, though simple, hardcoded the network matching in an ad hoc way.

Whether the change itself is worth it I don’t know. Highlighting the scam warning on testnet doesn’t really hurt.

bitcoin-core locked this on Aug 16, 2022

Un-color the scammers warning when using testnet. #387