Enhanced offset creation #407

yeastplume · 2020-05-13T16:19:18Z

Previously:

A single offset is chosen by whoever initiates the transaction. Their part of the excess is calculated as a combination of their inputs and outputs with the offset subtracted. The rest of the transaction continues with no more regard for the offset.

Now:

The offset is set to 0 at the beginning of the Transaction
Each party chooses their part of the offset before signing round 1 (which is when their contribution to the excess is calculated), and commits to ONLY their outputs minus their part of the offset as their excess (i.e, their inputs are not included in this total). Each new offset contribution is added to the total offset on the slate before being passed to the next party.
When inputs are added to the transaction, their blinding factors are subtracted from the offset to create a new offset.

With this change to how offsets and excesses are handled, we should be a stone's throw away from mostly lock-free transactions without actually having them implemented. This introduces the offset workflow while keeping the existing output locks in place, which should allow us to introduce lock-free transactions post 4.0.0 without breaking any compatibility since the offset/excess assumptions on either side of a transaction will be the same regardless of whether the outputs are pre-locked or chosen just before posting.

Note that quite a bit of the code is littered with is_compact conditionals since we still need to support 3.0.0 compatibility for a short period between release and HF3.

yeastplume · 2020-05-14T16:03:01Z

Ready for review with all tests passing and compatibility tested with 3.0.0 wallets.

libwallet/src/internal/selection.rs

tromp · 2020-05-14T19:36:40Z

libwallet/src/internal/selection.rs

+		// it will be adjusted when we add inputs
+		0 => BlindingFactor::zero(),
+		_ => slate.add_transaction_elements(keychain, &ProofBuilder::new(keychain), elems)?,
+	};


Why can't we pass empty elems to add_transaction_elements() ? (which I would also expect to yield BlindingFactor::zero())

Yes, I can move the check into add_transaction_elements, but the fundamental issue is the build module (contained in grin core) throws the error if elems is empty. The build library is some of the the oldest untouched part of the code and could perhaps do with another revision at some stage.

tromp · 2020-05-14T19:45:11Z

libwallet/src/internal/selection.rs

+				parts.push(build::coinbase_input(coin.value, coin.key_id.clone()));
+			} else {
+				parts.push(build::input(coin.value, coin.key_id.clone()));
+			}


This code suggests a method build::input(value, key_id, is_coinbase) to replace the current tow ?!

Similar to above, this is in the core tx build lib and could stand some revision (but after HF3)

tromp · 2020-05-14T19:52:05Z

libwallet/src/slate.rs

+			self.generate_offset(keychain, sec_key, use_test_rng)?;
+		}
+		// Always choose my part of the offset, and subtract from my excess
+		if self.is_compact() {


Avoid repeating tests by

if !self.is_compact() { if self.participant_data.len() == 0 { } } else { }

Btw, is there no method self.participant_data.is_empty() ? comparing len() with 0 looks ugly...

The logic is split here to allow for quick and easy deletion of 3.0.0 compatibility code once HF3 is behind us. There's a lot of this sprinkled around the code, it will be gone shortly after 4.0.0 is released.

tromp · 2020-05-14T19:54:50Z

libwallet/src/slate.rs

@@ -514,7 +518,7 @@ impl Slate {
 		// Generate a random kernel offset here
 		// and subtract it from the blind_sum so we create
 		// the aggsig context with the "split" key
-		self.tx_or_err_mut()?.offset = match use_test_rng {
+		let my_offset = match use_test_rng {


can't this be written as an if then else ? or doesn't that work at expression level?

Stylistic choice, I tend to prefer match statements over if-else (particularly when the block is returning a value) because I find there's less tendency for them to get confused once logic starts expanding

tromp · 2020-05-14T19:57:18Z

libwallet/src/slate.rs

+					.add_blinding_factor(self.offset.clone())
+					.add_blinding_factor(my_offset.clone()),
+			)?;
+			self.offset = total_offset;


is there no more compact way to express self.offset += my_offset ?
This looks awfully convoluted

No in the current code, but yes it's possible to implement operator traits to support this syntax. Again this is up in old portions of the grin Keychain code so a revision to the library could be done at a later date.

tromp · 2020-05-14T20:00:45Z

libwallet/src/slate.rs

-	if slate.offset != BlindingFactor::zero() {
-		tx.offset = slate.offset.clone()
+	if slate.off != BlindingFactor::zero() {
+		tx.offset = slate.off.clone()


What's the rationale for testing against zero? Is tx.offset already set to a nonzero value if slate.off == zero?

Left over from an earlier iteration, thanks for spotting.

tromp · 2020-05-14T20:02:04Z

libwallet/src/slate_versions/v4.rs

@@ -34,7 +34,8 @@
 //! *  The `payment_proof` struct is renamed to `proof`
 //! * The feat_args struct is added, which may be populated for non-Plain kernels
 //! * `proof` may be omitted from the slate if it is None (null),
-//! * `offset` is added, which may be optionally included during S3 and I3 to ensure the transaction can be re-built entirely from the slate information. Used for delayed transaction posting.
+//! * `off` (offset) is added, and will be modified by every participant in the transaction with a random
+//! value - the value of their inputs


not the value of their inputs, but their blinding factor.

tromp · 2020-05-14T22:24:43Z

I didn't notice anything wrong, but then I only looked at the surface level, lacking any detailed understanding of the code. My review then is that the changes all look plausible.

antiochp

I think this looks ok to me.
The pre/post HF3 logic definitely complicates things though.

yeastplume · 2020-05-18T09:39:56Z

I think this looks ok to me.
The pre/post HF3 logic definitely complicates things though.

Indeed it does, but I've tried to mark off and isolate any bits of code that can be removed after HF3, so hopefully this will become a lot cleaner.

* Add support for sending compact slates (#366) * WIP add support for sending compact slates * add repopulate_tx function to internal API * first pass at compacted slate working * move slate compaction to separate function * test fixes * support compact slate inits in invoice workflow * add compress flags to send and invoice * attempting to remove is_compact and assume all V4 slates begin as compact * attempting to calculate offsets when full tx data isn't available * update calc_commit to use participant blind data * update doctests for compact slates * start to remove unneeded fields from serialization * make num_participants optional * remove other_version from slate * use grin master branch * remove message field * lock height assumed to be 0 if it doesn't exist * don't serialise receiver signature when null * don't serialize payment_info if not needed * remove participant id from participant info * add note on id field * fix finalize and receive doctests * finalize_tx tests, init_send_tx tests * doctests for process_invoice_tx, retrieve_tx, tx_lock_outputs * finished test changes * update from grin master * rebuild PR from diff (#380) * recreate PR from diff (#381) * serialize tx struct into top level coms object (#382) * remove height (#383) * Add State Slate (#384) * add state field to slate and SlateV4 * set slate state at each transaction stage, add check to tests * serialize slate status properly * V4 Slate field tweaks (#386) * various tweaks to V4 slate * field renaming * serialize slate v4 ID as base64 (#387) * remove amount and fee where not needed (#388) * Final Changes for compact Slate (#389) * add tests for all types of file output, remove message args * default range proof serialization * shorten output features serialization * rename payment proof fields in slate v4 * v4 payment proof serialization * Binary Slates (#385) * start test implementation * add experimental binary serialization to slate * serialize id * serialize fields that can be skipped as a separate struct * factor out sigs serialization * clean up sigs and coms serialization * completed v4 bin serialization * add manual de/ser traits for V4 bin slate * add simple byte array serializer * complete wiring in of bin slate serialization * clarify comment * clarify comment * update version * test output dir name fix * update slate v4 change description * add binary output to command line * Remove unneeded signature data during S2 and I2 stages (#390) * remove unneeded return signature data during S2 * remove unneeded sig data from I2 * Doctest Fixes for compact slate branch (#392) * begin to fix doctests * more doctest fixes * fix receive_tx * update get_stored_tx to accept an UUID instead of a tx object, and operate on a raw Transaction object (#394) * Fixes to async transaction posting (#395) * unstash post_tx changes * add offset during S3 and I3 * Revert slate id serialization to hex-string uuid (#396) * update from master (#397) * v3.x.x - v4.0.0 wallet compatibility fixes (#398) * changes to support http sending to v3 wallets * sending via http/tor TO 3.0.0 wallet works * receiving FROM 3.0.0 wallets works over http/tor * output converted V3 slate when needed * paying invoices from 3.0.0 wallets working * handle all participant info in slate states * sending and receiving standard file transactions between v3 and 4 wallets confirmed working * all file-based workflows working * fixes resulting from tests * remove reminder warnings * remove lock_height, add kernel_features + arguments (#399) * grin-wallet master now building against grin master (#402) (#403) Co-authored-by: Antioch Peverell <apeverell@protonmail.com> * Enhanced offset creation (#407) * initial tests reworking offset creation * invoice flow fixing + tests * further test fixes * change offset name in v4 slate, base64 serialize * logic optimisation * changes based on review feedback Co-authored-by: Antioch Peverell <apeverell@protonmail.com>

* Add support for sending compact slates (mimblewimble#366) * WIP add support for sending compact slates * add repopulate_tx function to internal API * first pass at compacted slate working * move slate compaction to separate function * test fixes * support compact slate inits in invoice workflow * add compress flags to send and invoice * attempting to remove is_compact and assume all V4 slates begin as compact * attempting to calculate offsets when full tx data isn't available * update calc_commit to use participant blind data * update doctests for compact slates * start to remove unneeded fields from serialization * make num_participants optional * remove other_version from slate * use grin master branch * remove message field * lock height assumed to be 0 if it doesn't exist * don't serialise receiver signature when null * don't serialize payment_info if not needed * remove participant id from participant info * add note on id field * fix finalize and receive doctests * finalize_tx tests, init_send_tx tests * doctests for process_invoice_tx, retrieve_tx, tx_lock_outputs * finished test changes * update from grin master * rebuild PR from diff (mimblewimble#380) * recreate PR from diff (mimblewimble#381) * serialize tx struct into top level coms object (mimblewimble#382) * remove height (mimblewimble#383) * Add State Slate (mimblewimble#384) * add state field to slate and SlateV4 * set slate state at each transaction stage, add check to tests * serialize slate status properly * V4 Slate field tweaks (mimblewimble#386) * various tweaks to V4 slate * field renaming * serialize slate v4 ID as base64 (mimblewimble#387) * remove amount and fee where not needed (mimblewimble#388) * Final Changes for compact Slate (mimblewimble#389) * add tests for all types of file output, remove message args * default range proof serialization * shorten output features serialization * rename payment proof fields in slate v4 * v4 payment proof serialization * Binary Slates (mimblewimble#385) * start test implementation * add experimental binary serialization to slate * serialize id * serialize fields that can be skipped as a separate struct * factor out sigs serialization * clean up sigs and coms serialization * completed v4 bin serialization * add manual de/ser traits for V4 bin slate * add simple byte array serializer * complete wiring in of bin slate serialization * clarify comment * clarify comment * update version * test output dir name fix * update slate v4 change description * add binary output to command line * Remove unneeded signature data during S2 and I2 stages (mimblewimble#390) * remove unneeded return signature data during S2 * remove unneeded sig data from I2 * Doctest Fixes for compact slate branch (mimblewimble#392) * begin to fix doctests * more doctest fixes * fix receive_tx * update get_stored_tx to accept an UUID instead of a tx object, and operate on a raw Transaction object (mimblewimble#394) * Fixes to async transaction posting (mimblewimble#395) * unstash post_tx changes * add offset during S3 and I3 * Revert slate id serialization to hex-string uuid (mimblewimble#396) * update from master (mimblewimble#397) * v3.x.x - v4.0.0 wallet compatibility fixes (mimblewimble#398) * changes to support http sending to v3 wallets * sending via http/tor TO 3.0.0 wallet works * receiving FROM 3.0.0 wallets works over http/tor * output converted V3 slate when needed * paying invoices from 3.0.0 wallets working * handle all participant info in slate states * sending and receiving standard file transactions between v3 and 4 wallets confirmed working * all file-based workflows working * fixes resulting from tests * remove reminder warnings * remove lock_height, add kernel_features + arguments (mimblewimble#399) * grin-wallet master now building against grin master (mimblewimble#402) (mimblewimble#403) Co-authored-by: Antioch Peverell <apeverell@protonmail.com> * Enhanced offset creation (mimblewimble#407) * initial tests reworking offset creation * invoice flow fixing + tests * further test fixes * change offset name in v4 slate, base64 serialize * logic optimisation * changes based on review feedback Co-authored-by: Antioch Peverell <apeverell@protonmail.com>

yeastplume added 4 commits May 13, 2020 16:59

initial tests reworking offset creation

49457f0

invoice flow fixing + tests

3ab0b45

further test fixes

cefcb87

change offset name in v4 slate, base64 serialize

3de39c6

yeastplume changed the title ~~[WIP] Enhanced offset creation~~ Enhanced offset creation May 14, 2020

yeastplume requested review from antiochp and tromp May 14, 2020 16:01

yeastplume requested a review from DavidBurkett May 14, 2020 16:05

tromp reviewed May 14, 2020

View reviewed changes

libwallet/src/internal/selection.rs Outdated Show resolved Hide resolved

logic optimisation

49d1334

tromp reviewed May 14, 2020

View reviewed changes

changes based on review feedback

abdea30

antiochp approved these changes May 16, 2020

View reviewed changes

yeastplume merged commit fabe41a into mimblewimble:4.0.0/compact_slates May 18, 2020

This was referenced May 18, 2020

Improve Offset Selection #405

Closed

Release Planning: Grin v4.0.0 mimblewimble/grin-pm#248

Closed

yeastplume deleted the offset_selection branch July 13, 2020 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced offset creation #407

Enhanced offset creation #407

yeastplume commented May 13, 2020 •

edited

yeastplume commented May 14, 2020

tromp May 14, 2020

yeastplume May 15, 2020

tromp May 14, 2020

yeastplume May 15, 2020

tromp May 14, 2020 •

edited

yeastplume May 15, 2020

tromp May 14, 2020

yeastplume May 15, 2020

tromp May 14, 2020 •

edited

yeastplume May 15, 2020

tromp May 14, 2020

yeastplume May 15, 2020

tromp May 14, 2020

yeastplume May 15, 2020

tromp commented May 14, 2020

antiochp left a comment

yeastplume commented May 18, 2020

Enhanced offset creation #407

Enhanced offset creation #407

Conversation

yeastplume commented May 13, 2020 • edited

yeastplume commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromp May 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromp May 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromp commented May 14, 2020

antiochp left a comment

Choose a reason for hiding this comment

yeastplume commented May 18, 2020

yeastplume commented May 13, 2020 •

edited

tromp May 14, 2020 •

edited

tromp May 14, 2020 •

edited