Add a menu option and dialog to verify a signed message #906

sje397 · 2012-02-27T11:58:08Z

Being able to sign a message without the ability to verify signed messages is a little lopsided :)

luke-jr · 2012-02-27T22:34:33Z

This doesn't respect FIRST_CLASS_MESSAGING=1
Instead of a new dialog/tab, perhaps it would make sense to just add a button to the Sign Message one (and rename it Messages or something)?

sje397 · 2012-02-27T23:13:30Z

I tried modifying the existing window initially. It was ugly. The address field should be below the message when verifying, and having lots of hiding/showing components feels like a design problem to me - too much responsibility for a single UI class that really should be separated.

I treated 'FIRST_CLASS_MESSAGING' as relating to signing messages only. As per above, I think the UI should be separate and I don't think subtabs make sense in a first-class messaging window...and I don't think having two tabs for messaging in the bitcoin UI is a fair waiting given the main purpose. Basically, I don't think 'FIRST_CLASS_MESSAGING' makes any sense :)

luke-jr · 2012-02-27T23:21:19Z

How about moving the message to the top, then have the address and signature under that, with "Sign" and "Verify" buttons between the addr and sig textlines, with arrows of which direction it processes?

sje397 · 2012-02-28T02:48:48Z

That sounds pretty good to me. I'll mess around with it this evening.

sje397 · 2012-02-28T11:13:17Z

I still don't like it. It means having the signature line edit writeable when you may just want to sign and have that generated...and it means having the address book and paste buttons enabled when you might just want to validate. It also means the order of elements on the page is not necessarily what you want to see (i.e. to verify, you fill in message, then skip address, then fill in signature). Plus there are other ugly issues with the way things are done now - e.g. select one of your own addresses, click sign message, click address book (thereby opening a window containing the same list of addresses you just left) and there is a sign message button again, which does nothing perceptible.

I would rather leave the message functionality in a menu somewhere (maybe the same should be true of the qrcode generation function) so that it doesn't bother users that just want the basics, and keep the sign and validate UI elements simple.

luke-jr · 2012-03-09T02:14:30Z

While validation is more of an advanced tool, signing is basics. Perhaps a UI expert should provide input on how to best integrate them together.

laanwj · 2012-03-10T18:33:14Z

Good, indeed it was asymmetric to have signing without verifying, this at least makes it complete.

IMO you don't have to do any extra work to make it play nice with FIRST_CLASS_MESSAGING. Only @luke-jr uses that option, let him do that :)

TheBlueMatt · 2012-03-12T16:39:20Z

Why do we have a FIRST_CLASS_MESSAGING if no one uses it, also, it needs a much more descriptive name, FIRST_CLASS_MESSAGING doesnt say anything about what it actually does.

luke-jr · 2012-03-12T16:43:47Z

"No one" uses 0.6 yet. It makes messaging first-class.

laanwj · 2012-03-12T17:11:44Z

@TheBlueMatt we have FIRST_CLASS_MESSAGING because it was part of the signmessage pull. Luke-jr wanted to add signing messages as a separate tab, whereas according to us there are already too many tabs and we should reserve them for essential functionality. So he put it behind a qmake option, I guess because he wants to keep it as a tab himself...

TheBlueMatt · 2012-03-12T17:15:17Z

Mmm, so maybe it should be SIGNMESSAGE_IN_TAB or something that actually describes what it does?

luke-jr · 2012-03-12T18:00:32Z

BTW: If Samuel's GUI gets merged, the "too many tabs" excuse goes away, as there's plenty of room.

laanwj · 2012-03-12T18:31:24Z

It's not about room (if you change the toolbars to icon-only you also have enough space). It's about not confusing users.

luke-jr · 2012-04-10T13:44:00Z

Rebasing required.

sje397 · 2012-04-10T14:27:30Z

Rebased.

Diapolo · 2012-04-22T15:07:30Z

Can anyone post a screenshot, I would like to know where the button is loctated and how it looks currently?

sje397 · 2012-04-22T15:27:19Z

I rebased and fixed a tiny issue due to a change in the CDataStream constructor. I also removed that HTML.

I'm trying to get a screen shot but wrestling with a new install of arch.

laanwj · 2012-05-05T07:24:04Z

Great to have this, isn't symmetry a beautiful thing.

One suggestion: Please make this non-modal, like the other utility windows.
(that'll also make testing easier as we can keep open a "sign message" and "verify message" window open at the same time)

Diapolo · 2012-05-05T09:18:05Z

I would like to know, where in the menu this is placed, as I would suggest to have it below Sign message (in file) or in a to add tools menu.

sje397 · 2012-05-11T13:44:00Z

The menu option is underneath 'Sign Message' in File, yes. I couldn't get a screenshot because the menu being shown disabled my print-screen key :(

I've rebased, and made the dialog non-modal. I also moved the call to 'setAttribute(Qt::WA_DeleteOnClose)' out of the QRCodeDialog to match how it is handled here. This is better since that decision should be up to the creator of the dialog rather than the dialog itself.

Diapolo · 2012-05-11T13:47:30Z

src/qt/forms/verifymessagedialog.ui

+      <string extracomment="testing"/>
+     </property>
+     <property name="html">
+      <string>&lt;!DOCTYPE HTML PUBLIC &quot;-//W3C//DTD HTML 4.0//EN&quot; &quot;http://www.w3.org/TR/REC-html40/strict.dtd&quot;&gt;


And once more we have HTML :D.

I tried removing that, but the editor just puts it back in. I'll try hand-editing and see what happens...but I'd like to avoid a situation where people have to manually edit the file if they touch it.

Can you set this to plainText? Perhaps this helps? Or can you use another GUI element that does the same?
The messagepage uses an QPlainTextEdit field and I would prefer them to be even in terms of GUI element usage.

sje397 · 2012-05-11T14:03:14Z

Changed to QPlainTextEdit (on Diapolo's advice), found the button I'd lost to reset the checkboxes so the 'italic false' stuff should be gone too.

Diapolo · 2012-05-11T14:06:09Z

You have to excuse me, as I'm kind of a perfectionist ^^, I even care about the smallest things ... sometimes this is bad, but more often it's a good thing :D.

sje397 · 2012-05-11T14:09:35Z

No, that's a very good thing. I'd rather end up with better software too :)

Diapolo · 2012-05-11T14:11:44Z

src/qt/forms/verifymessagedialog.ui

+        <string>Copy the currently selected address to the system clipboard</string>
+       </property>
+       <property name="text">
+        <string>&amp;Copy to Clipboard</string>


If this copies the resulting address, please use copy "Copy Address" as text.

(Also move 'setAttribute(Qt::WA_DeleteOnClose)' out of QRCodeDialog)

Add a menu option and dialog to verify a signed message

ca207f6 Improve CreateDenominated and MakeCollateralAmounts: - should use funds only from the same receiving address - should denominate while has funds but has not enough denoms - slightly refactored 20d3a23 Add few more improvements to CreateDenominated and MakeCollateralAmounts: - return change to the origin address to use it in future splits - fix a bug with incorrect overshoot - more debug (optional/errors only) and fixed var names

…t-switch Define for CURRENCY_UNIT is no longer needed

ca2fd10 use non-Cellar OpenSSL path for macOS (Fuzzbawls) 69b0032 fix config.h include file name (Fuzzbawls) d413496 [Build] Initial CMake support (Fuzzbawls) Tree-SHA512: 459d881a6128027c1b1fe6df13b0dce5fe484af377afece74ccc135a5e5ea320496165ffbdef70d806bd879be7cad39d4bfc3cdffbacfc2a61d0c8e53265b0ab

efad350 Merge bitcoin#906: Use modified divsteps with initial delta=1/2 for constant-time cc2c09e Merge bitcoin#918: Clean up configuration in gen_context 0706796 add ECMULT_GEN_PREC_BITS to basic_config.h a3aa262 gen_context: Don't include basic-config.h be0609f Add unit tests for edge cases with delta=1/2 variant of divsteps cd393ce Optimization: only do 59 hddivsteps per iteration instead of 62 277b224 Use modified divsteps with initial delta=1/2 for constant-time 376ca36 Fix typo in explanation 1e5d50f Merge bitcoin#889: fix uninitialized read in tests c083cc6 Merge bitcoin#903: Make argument of fe_normalizes_to_zero{_var} const 6e89853 Merge bitcoin#907: changed import to use brackets <> for openssl 4504472 changed import to use brackets <> for openssl as they are not local to the project 26de4df Merge bitcoin#831: Safegcd inverses, drop Jacobi symbols, remove libgmp 23c3fb6 Make argument of fe_normalizes_to_zero{_var} const 24ad04f Make scalar_inverse{,_var} benchmark scale with SECP256K1_BENCH_ITERS ebc1af7 Optimization: track f,g limb count and pass to new variable-time update_fg_var b306935 Optimization: use formulas instead of lookup tables for cancelling g bits 9164a1b Optimization: special-case zero modulus limbs in modinv64 1f233b3 Remove num/gmp support 20448b8 Remove unused Jacobi symbol support 5437e7b Remove unused scalar_sqr aa9cc52 Improve field/scalar inverse tests 1e0e885 Make field/scalar code use the new modinv modules for inverses 436281a Move secp256k1_fe_inverse{_var} to per-impl files aa404d5 Move secp256k1_scalar_{inverse{_var},is_even} to per-impl files 08d5496 Improve bounds checks in modinv modules 151aac0 Add tests for modinv modules d8a92fc Add extensive comments on the safegcd algorithm and implementation 8e415ac Add safegcd based modular inverse modules de0a643 Add secp256k1_ctz{32,64}_var functions 4c3ba88 Merge bitcoin#901: ci: Switch all Linux builds to Debian and more improvements 9361f36 ci: Select number of parallel make jobs depending on CI environment 28eccdf ci: Split output of logs into multiple sections c7f754f ci: Run PRs on merge result instead of on the source branch b994a8b ci: Print information about binaries using "file" f24e122 ci: Switch all Linux builds to Debian ebdba03 Merge bitcoin#891: build: Add workaround for automake 1.13 and older 3a8b47b Merge bitcoin#894: ctime_test: move context randomization test to the end 7d3497c ctime_test: move context randomization test to the end 99a1cfe print warnings for conditional-uninitialized 3d2cf6c initialize variable in tests f329bba build: Add workaround for automake 1.13 and older 24d1656 Merge bitcoin#882: Use bit ops instead of int mult for constant-time logic in gej_add_ge e491d06 Use bit ops instead of int mult for constant-time logic in gej_add_ge f8c0b57 Merge bitcoin#864: Add support for Cirrus CI cc2a545 ci: Refactor Nix shell files 2480e55 ci: Remove support for Travis CI 2b359f1 ci: Enable simple cache for brewing valgrind on macOS 8c02e46 ci: Add support for Cirrus CI 659d0d4 Merge bitcoin#880: Add parens around ROUND_TO_ALIGN's parameter. b6f6498 Add parens around ROUND_TO_ALIGN's parameter. This makes the macro robust against a hypothetical ROUND_TO_ALIGN(foo ? sizeA : size B) invocation. a4abaab Merge bitcoin#877: Add missing secp256k1_ge_set_gej_var decl. 5671e5f Merge bitcoin#874: Remove underscores from header defs. db72678 Merge bitcoin#878: Remove unused secp256k1_fe_inv_all_var b732701 Merge bitcoin#875: Avoid casting (void**) values. 75d2ae1 Remove unused secp256k1_fe_inv_all_var 482e4a9 Add missing secp256k1_ge_set_gej_var decl. 2730618 Avoid casting (void**) values. Replaced with an expression that only casts (void*) values. fb390c5 Remove underscores from header defs. This makes them consistent with other files and avoids reserved identifiers. f2d9aea Merge bitcoin#862: Autoconf improvements 328aaef Merge bitcoin#845: Extract the secret key from a keypair 3c15130 Improve CC_FOR_BUILD detection 47802a4 Restructure and tidy configure.ac 252c19d Ask brew for valgrind include path 8c727b9 Merge bitcoin#860: fixed trivial typo b7bc3a4 fixed typo 33cb3c2 Add secret key extraction from keypair to constant time tests 36d9dc1 Add seckey extraction from keypair to the extrakeys tests fc96aa7 Add a function to extract the secretkey from a keypair 98dac87 Merge bitcoin#858: Fix insecure links 07aa4c7 Fix insecure links b61f9da Merge bitcoin#857: docs: fix simple typo, dependecy -> dependency 18aadf9 docs: fix simple typo, dependecy -> dependency 2d9e717 Merge bitcoin#852: Add sage script for generating scalar_split_lambda constants dc6e5c3 Merge bitcoin#854: Rename msg32 to msghash32 in ecdsa_sign/verify and add explanation 6e85d67 Rename tweak to tweak32 in public API f587f04 Rename msg32 to msghash32 in ecdsa_sign/verify and add explanation 329a2e0 sage: Add script for generating scalar_split_lambda constants 8f0c6f1 Merge bitcoin#851: make test count iteration configurable by environment variable f4fa8d2 forbid a test iteration of 0 or less f554dfc sage: Reorganize files 3a10696 Merge bitcoin#849: Convert Sage code to Python 3 (as used by Sage >= 9) 13c88ef Convert Sage code to Python 3 (as used by Sage >= 9) 0ce4554 make test count iteration configurable by environment variable 9e5939d Merge bitcoin#835: Don't use reserved identifiers memczero and benchmark_verify_t d0a83f7 Merge bitcoin#839: Prevent arithmetic on NULL pointer if the scratch space is too small 903b16a Merge bitcoin#840: Return NULL early in context_preallocated_create if flags invalid 1f4dd03 Typedef (u)int128_t only when they're not provided by the compiler ebfa205 Return NULL early in context_preallocated_create if flags invalid 29a299e Run the undefined behaviour sanitizer on Travis 7506e06 Prevent arithmetic on NULL pointer if the scratch space is too small e89278f Don't use reserved identifiers memczero and benchmark_verify_t git-subtree-dir: src/secp256k1 git-subtree-split: efad350

Diapolo reviewed May 11, 2012
View reviewed changes

Add a menu option to verify a signed message

0c58793

(Also move 'setAttribute(Qt::WA_DeleteOnClose)' out of QRCodeDialog)

laanwj added a commit that referenced this pull request May 14, 2012

Merge pull request #906 from sje397/ValidateMessage

bb361cc

Add a menu option and dialog to verify a signed message

laanwj merged commit bb361cc into bitcoin:master May 14, 2012

coblee referenced this pull request in litecoin-project/litecoin Jul 17, 2012

Merge pull request #906 from sje397/ValidateMessage

38fab7c

Add a menu option and dialog to verify a signed message

ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Jan 17, 2018

Merge pull request bitcoin#906 from Greg-Griffith/remove-Currency_uni…

62d36dd

…t-switch Define for CURRENCY_UNIT is no longer needed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add a menu option and dialog to verify a signed message #906

Add a menu option and dialog to verify a signed message #906

Uh oh!

Conversation

sje397 commented Feb 27, 2012

Uh oh!

luke-jr commented Feb 27, 2012

Uh oh!

sje397 commented Feb 27, 2012

Uh oh!

luke-jr commented Feb 27, 2012

Uh oh!

sje397 commented Feb 28, 2012

Uh oh!

sje397 commented Feb 28, 2012

Uh oh!

luke-jr commented Mar 9, 2012

Uh oh!

laanwj commented Mar 10, 2012

Uh oh!

TheBlueMatt commented Mar 12, 2012

Uh oh!

luke-jr commented Mar 12, 2012

Uh oh!

laanwj commented Mar 12, 2012

Uh oh!

TheBlueMatt commented Mar 12, 2012

Uh oh!

luke-jr commented Mar 12, 2012

Uh oh!

laanwj commented Mar 12, 2012

Uh oh!

luke-jr commented Apr 10, 2012

Uh oh!

sje397 commented Apr 10, 2012

Uh oh!

Diapolo commented Apr 22, 2012

Uh oh!

sje397 commented Apr 22, 2012

Uh oh!

laanwj commented May 5, 2012

Uh oh!

Diapolo commented May 5, 2012

Uh oh!

sje397 commented May 11, 2012

Uh oh!

Diapolo May 11, 2012

Choose a reason for hiding this comment

Uh oh!

sje397 May 11, 2012

Choose a reason for hiding this comment

Uh oh!

Diapolo May 11, 2012

Choose a reason for hiding this comment

Uh oh!

sje397 commented May 11, 2012

Uh oh!

Diapolo commented May 11, 2012

Uh oh!

sje397 commented May 11, 2012

Uh oh!

Diapolo May 11, 2012

Choose a reason for hiding this comment

Uh oh!

Uh oh!