added pow to text notes, reactions and metadata events.
noxy is now only used if event has valid work proof.
updates https://git.qcode.ch/nostr/nostrweb/issues/51
good start! i would also mention that nostrweb now does pow in the readme and about page so users know what's going on.
also, made an issue in nostr-tools repo: https://github.com/fiatjaf/nostr-tools/issues/63
would add a comment why 16, even if it's arbitrary:
// arbitrary difficulty, still experimenting.
// measured empirically, takes N sec on average to mine a text note event.
another idea for the future: make it adjustable? i'd imagine a slider in profile settings.
would add a comment why 16, even if it's arbitrary:
```
// arbitrary difficulty, still experimenting.
// measured empirically, takes N sec on average to mine a text note event.
```
another idea for the future: make it adjustable? i'd imagine a slider in profile settings.
i would add this tag in the powEvent function. otherwise, you need to insert it everywhere. it's also easier to read and verify correctness when pow-related things are in the single function imho:
functionpowEvent(evt,difficulty){constmax=256;// arbitrary
if(!Number.isInteger(difficulty)||difficulty<0||difficulty>max){thrownewError(`difficulty must be an integer between 0 and ${max}`);}evt.tags.push(['nonce','0',`${difficulty}`]// continue with mining
// ...
}
i would add this tag in the `powEvent` function. otherwise, you need to insert it everywhere. it's also easier to read and verify correctness when pow-related things are in the single function imho:
```js
function powEvent(evt, difficulty) {
const max = 256; // arbitrary
if (!Number.isInteger(difficulty) || difficulty < 0 || difficulty > max) {
throw new Error(`difficulty must be an integer between 0 and ${max}`);
}
evt.tags.push(['nonce', '0', `${difficulty}`]
// continue with mining
// ...
}
```
is it possible to make it async? i'm hoping when invoked as await powEvent(...) it UI thread in browser's javascript won't be blocked.
also, i think it needs a timeout arg, to run at most that long. otherwise, it could run indefinitely and go crazy with the cpu fan :)
another idea is to run pow in a web worker. i imagine something like this:
/**
* run proof of work in a worker until at least the specified difficulty.
* if succcessful, the returned event contains the 'nonce' tag
* and the updated created_at timestamp.
*
* powEvent returns a rejected promise if the funtion runs for longer than timeout.
* a zero timeout makes mineEvent run without a time limit.
*/functionpowEvent(event,difficulty,timeout){returnnewPromise((resolve,reject)=>{constwebWorkerURL=URL.createObjectURL(newBlob(['(',powEventWorker(),')()'],{type:'application/javascript'}));constworker=newWorker(webWorkerURL);worker.onmessage=(msg)=>{worker.terminate();if(msg.error){reject(msg.error);}else{resolve(msg.event);}};worker.onerror=(err)=>{worker.terminate();reject(err);};worker.postMessage({event,difficulty,timeout});URL.revokeObjectURL(webWorkerURL);// one-time worker; no longer need the URL obj
});}functionpowEventWorker(){return()=>{functionmine(event,difficulty,timeout){// do the actual mining here
// ...
// throw err if running longer than the timeout
}addEventListener('message',async(msg)=>{letnostrEvent=msg.data.event;constdifficulty=msg.data.difficulty;consttimeout=msg.data.timeout;try{constminedEvent=mine(nostrEvent,difficulty,timeout);postMessage({event:minedEvent});}catch(err){postMessage({error:err});}});}}
is it possible to make it async? i'm hoping when invoked as `await powEvent(...)` it UI thread in browser's javascript won't be blocked.
also, i think it needs a `timeout` arg, to run at most that long. otherwise, it could run indefinitely and go crazy with the cpu fan :)
another idea is to run pow in a web worker. i imagine something like this:
```js
/**
* run proof of work in a worker until at least the specified difficulty.
* if succcessful, the returned event contains the 'nonce' tag
* and the updated created_at timestamp.
*
* powEvent returns a rejected promise if the funtion runs for longer than timeout.
* a zero timeout makes mineEvent run without a time limit.
*/
function powEvent(event, difficulty, timeout) {
return new Promise((resolve, reject) => {
const webWorkerURL = URL.createObjectURL(new Blob(['(', powEventWorker(), ')()'], {type: 'application/javascript'}));
const worker = new Worker(webWorkerURL);
worker.onmessage = (msg) => {
worker.terminate();
if (msg.error) {
reject(msg.error);
} else {
resolve(msg.event);
}
};
worker.onerror = (err) => {
worker.terminate();
reject(err);
};
worker.postMessage({event, difficulty, timeout});
URL.revokeObjectURL(webWorkerURL); // one-time worker; no longer need the URL obj
});
}
function powEventWorker() {
return () => {
function mine(event, difficulty, timeout) {
// do the actual mining here
// ...
// throw err if running longer than the timeout
}
addEventListener('message', async (msg) => {
let nostrEvent = msg.data.event;
const difficulty = msg.data.difficulty;
const timeout = msg.data.timeout;
try {
const minedEvent = mine(nostrEvent, difficulty, timeout);
postMessage({event: minedEvent});
} catch (err) {
postMessage({error: err});
}
});
}
}
```
I played around with the worker a bit, unfortunately it seems passing a worker by URL.createObjectURL doesn't include getEventHash by nostr-tools, this is why I added a worker.js file.
I played around with the worker a bit, unfortunately it seems passing a worker by URL.createObjectURL doesn't include `getEventHash` by `nostr-tools`, this is why I added a worker.js file.
wait, i'm confused. why does `getEventHash` need a private key? here it takes only the `Event`: https://github.com/nbd-wtf/nostr-tools/blob/4b36848b/event.ts#L56
maybe we're using an older version?
i'm afraid this results in exponent format for large `n`:
```
>> `${2**128}`
<- "3.402823669209385e+38"
```
but we always want regular format based 10, right?
```
>> `${BigInt(2**128).toString()}`
<- "340282366920938463463374607431768211456"
```
before hashing the event, i believe you'll want to update the created_at timestamp. from nip-13:
It is recommended to update the created_at as well during this process.
before hashing the event, i believe you'll want to update the `created_at` timestamp. from [nip-13](https://github.com/nostr-protocol/nips/blob/master/13.md):
> It is recommended to update the created_at as well during this process.
so, we have 8 + 8 + 8 + 8 + 4 = 36 leading 0 bits but the current impl will count only 8*4 = 32.
i saw you difficulty / 8 to make it round but i think it's relatively easy to do it at the bit level. played around a bit. here's a version i came up with that counts exact number:
/**
* evaluate the difficulty of hex32 according to nip-13.
* @param hex32 a string of 64 chars - 32 bytes in hex representation
*/functionzeroLeadingBitsCount(hex32){letcount=0;for(leti=0;i<64;i+=2){consthexbyte=hex32.slice(i,i+2);// grab next byte
if(hexbyte=='00'){count+=8;continue;}// reached non-zero byte; count number of 0 bits in hexbyte
constbits=parseInt(hexbyte,16).toString(2).padStart(8,'0');for(letb=0;b<8;b++){if(bits[b]=='1'){break;// reached non-zero bit; stop
}count+=1;}break;}returncount;}
and here's another version. it looks simpler but i don't know which one is faster. it should be possible to benchmark cpu and memory performance, and pick the best one?
/**
* evaluate the difficulty of hex32 according to nip-13.
* @param hex32 a string of 64 chars - 32 bytes in hex representation
*/functionzeroLeadingBitsCount(hex32){constbits=parseInt(hex32,16).toString(2).padStart(8*32,'0');letcount=0;for(leti=0;i<8*32;i++){if(bits[i]=='1'){break;}count+=1;}returncount;}
this unfortunately won't count the exact number of bits in all cases. taking the event id from [nip-13](https://github.com/nostr-protocol/nips/blob/master/13.md) example, it's binary representation is:
```
>> const id = '000000000e9d97a1ab09fc381030b346cdd7a142ad57e6df0b46dc9bef6c7e2d';
>> parseInt(id, 16).toString(2).padStart(8*32, '0')
<- '0000000000000000000000000000000000001110100111...
```
or for better readability:
```
00000000 00000000 00000000 00000000 00001110 100111...
```
so, we have `8 + 8 + 8 + 8 + 4 = 36` leading 0 bits but the current impl will count only `8*4 = 32`.
i saw you `difficulty / 8` to make it round but i think it's relatively easy to do it at the bit level. played around a bit. here's a version i came up with that counts exact number:
```js
/**
* evaluate the difficulty of hex32 according to nip-13.
* @param hex32 a string of 64 chars - 32 bytes in hex representation
*/
function zeroLeadingBitsCount(hex32) {
let count = 0;
for (let i = 0; i < 64; i += 2) {
const hexbyte = hex32.slice(i, i+2); // grab next byte
if (hexbyte == '00') {
count += 8;
continue;
}
// reached non-zero byte; count number of 0 bits in hexbyte
const bits = parseInt(hexbyte, 16).toString(2).padStart(8, '0');
for (let b = 0; b < 8; b++) {
if (bits[b] == '1' ) {
break; // reached non-zero bit; stop
}
count += 1;
}
break;
}
return count;
}
```
and here's another version. it looks simpler but i don't know which one is faster. it should be possible to benchmark cpu and memory performance, and pick the best one?
```js
/**
* evaluate the difficulty of hex32 according to nip-13.
* @param hex32 a string of 64 chars - 32 bytes in hex representation
*/
function zeroLeadingBitsCount(hex32) {
const bits = parseInt(hex32, 16).toString(2).padStart(8*32, '0');
let count = 0;
for (let i = 0; i < 8*32; i++) {
if (bits[i] == '1') {
break;
}
count += 1;
}
return count;
}
```
there's a const difficulty = 16; elsewhere in main.js. i think you'll want to compare with it here?
maybe a better way to name the const is something like const acceptedDifficulty = 16 so that it reads more clear here:
if(difficulty2<acceptedDifficulty){returnfalse}
there's a `const difficulty = 16;` elsewhere in main.js. i think you'll want to compare with it here?
maybe a better way to name the const is something like `const acceptedDifficulty = 16` so that it reads more clear here:
```js
if (difficulty2 < acceptedDifficulty) {
return false
}
```
looks like that return assumes a difficulty is always divisible by 4 or 8 but it isn't always the case. could be 3, 7, 9, 11... plus, this is only commitment which can be fake.
i believe you'll want to use zeroLeadingBitsCount here too. for example:
functionvalidatePow(evt){// acceptedDifficulty is set by the user.
// let the event pass if user doesn't care about pow difficulty.
if(!acceptedDifficulty){returntrue;}consttag=evt.tags.find(tag=>tag[0]==='nonce');constcommittedDifficulty=tag&&parseInt(tag[2]);// discard events which committed to lower difficulty.
if(!committedDifficulty||committedDifficulty<acceptedDifficulty){returnfalse;}// difficulty commitment might be fake.
// the final check is the actual event pow difficulty.
returnzeroLeadingBitsCount(evt.ID)>=acceptedDifficulty;}
looks like that return assumes a difficulty is always divisible by 4 or 8 but it isn't always the case. could be 3, 7, 9, 11... plus, this is only commitment which can be fake.
i believe you'll want to use `zeroLeadingBitsCount` here too. for example:
```js
function validatePow(evt) {
// acceptedDifficulty is set by the user.
// let the event pass if user doesn't care about pow difficulty.
if (!acceptedDifficulty) {
return true;
}
const tag = evt.tags.find(tag => tag[0] === 'nonce');
const committedDifficulty = tag && parseInt(tag[2]);
// discard events which committed to lower difficulty.
if (!committedDifficulty || committedDifficulty < acceptedDifficulty) {
return false;
}
// difficulty commitment might be fake.
// the final check is the actual event pow difficulty.
return zeroLeadingBitsCount(evt.ID) >= acceptedDifficulty;
}
```
one can get luckly and produce a higher difficulty. so, the check should be an inequality:
if(zeroLeadingBitsCount(id)>=difficulty){// done: reached at least the desired difficulty, but maybe higher
returnevent;}
one can get luckly and produce a higher difficulty. so, the check should be an inequality:
```js
if (zeroLeadingBitsCount(id) >= difficulty) {
// done: reached at least the desired difficulty, but maybe higher
return event;
}
```
The nip-13 has difficulty commitment target / lucky protection :)
> This allows clients to protect against situations where bulk spammers targeting a lower difficulty get lucky and match a higher difficulty.
https://github.com/nostr-protocol/nips/blob/master/13.md#mining
I tried modifying go-nostr and added ErrDifficultyTooHigh, but somehow the tests always pass with go test or go test -run nip13_test.go (even if I modify the test so it should fail), doing something wrong 🤔
diff --git a/nip13/nip13.go b/nip13/nip13.go
index 35a7d1f..ef88368 100644
--- a/nip13/nip13.go
+++ b/nip13/nip13.go
@@ -14,6 +14,7 @@ import (
var (
ErrDifficultyTooLow = errors.New("nip13: insufficient difficulty")
+ ErrDifficultyTooHigh = errors.New("nip13: does not match committed difficulty target")
ErrGenerateTimeout = errors.New("nip13: generating proof of work took too long")
)
@@ -43,10 +44,13 @@ func Difficulty(eventID string) int {
// Note that Check performs no validation other than counting leading zero bits
// in an event ID. It is up to the callers to verify the event with other methods,
// such as [nostr.Event.CheckSignature].
-func Check(eventID string, minDifficulty int) error {
- if Difficulty(eventID) < minDifficulty {
+func Check(eventID string, targetDifficulty int) error {
+ if Difficulty(eventID) < targetDifficulty {
return ErrDifficultyTooLow
}
+ if Difficulty(eventID) > targetDifficulty {
+ return ErrDifficultyTooHigh
+ }
return nil
}
diff --git a/nip13/nip13_test.go b/nip13/nip13_test.go
index b1e4409..cc1ae6a 100644
--- a/nip13/nip13_test.go
+++ b/nip13/nip13_test.go
@@ -13,20 +13,20 @@ import (
func TestCheck(t *testing.T) {
const eventID = "000000000e9d97a1ab09fc381030b346cdd7a142ad57e6df0b46dc9bef6c7e2d"
tests := []struct {
- minDifficulty int
+ difficulty int
wantErr error
}{
- {-1, nil},
- {0, nil},
- {1, nil},
- {35, nil},
+ {-1, ErrDifficultyTooHigh},
+ {0, ErrDifficultyTooHigh},
+ {1, ErrDifficultyTooHigh},
+ {35, ErrDifficultyTooHigh},
{36, nil},
{37, ErrDifficultyTooLow},
{42, ErrDifficultyTooLow},
}
for i, tc := range tests {
- if err := Check(eventID, tc.minDifficulty); err != tc.wantErr {
- t.Errorf("%d: Check(%q, %d) returned %v; want err: %v", i, eventID, tc.minDifficulty, err, tc.wantErr)
+ if err := Check(eventID, tc.difficulty); err != tc.wantErr {
+ t.Errorf("%d: Check(%q, %d) returned %v; want err: %v", i, eventID, tc.difficulty, err, tc.wantErr)
}
}
}
I tried modifying `go-nostr` and added ErrDifficultyTooHigh, but somehow the tests always pass with `go test` or `go test -run nip13_test.go` (even if I modify the test so it should fail), doing something wrong 🤔
```diff
diff --git a/nip13/nip13.go b/nip13/nip13.go
index 35a7d1f..ef88368 100644
--- a/nip13/nip13.go
+++ b/nip13/nip13.go
@@ -14,6 +14,7 @@ import (
var (
ErrDifficultyTooLow = errors.New("nip13: insufficient difficulty")
+ ErrDifficultyTooHigh = errors.New("nip13: does not match committed difficulty target")
ErrGenerateTimeout = errors.New("nip13: generating proof of work took too long")
)
@@ -43,10 +44,13 @@ func Difficulty(eventID string) int {
// Note that Check performs no validation other than counting leading zero bits
// in an event ID. It is up to the callers to verify the event with other methods,
// such as [nostr.Event.CheckSignature].
-func Check(eventID string, minDifficulty int) error {
- if Difficulty(eventID) < minDifficulty {
+func Check(eventID string, targetDifficulty int) error {
+ if Difficulty(eventID) < targetDifficulty {
return ErrDifficultyTooLow
}
+ if Difficulty(eventID) > targetDifficulty {
+ return ErrDifficultyTooHigh
+ }
return nil
}
diff --git a/nip13/nip13_test.go b/nip13/nip13_test.go
index b1e4409..cc1ae6a 100644
--- a/nip13/nip13_test.go
+++ b/nip13/nip13_test.go
@@ -13,20 +13,20 @@ import (
func TestCheck(t *testing.T) {
const eventID = "000000000e9d97a1ab09fc381030b346cdd7a142ad57e6df0b46dc9bef6c7e2d"
tests := []struct {
- minDifficulty int
+ difficulty int
wantErr error
}{
- {-1, nil},
- {0, nil},
- {1, nil},
- {35, nil},
+ {-1, ErrDifficultyTooHigh},
+ {0, ErrDifficultyTooHigh},
+ {1, ErrDifficultyTooHigh},
+ {35, ErrDifficultyTooHigh},
{36, nil},
{37, ErrDifficultyTooLow},
{42, ErrDifficultyTooLow},
}
for i, tc := range tests {
- if err := Check(eventID, tc.minDifficulty); err != tc.wantErr {
- t.Errorf("%d: Check(%q, %d) returned %v; want err: %v", i, eventID, tc.minDifficulty, err, tc.wantErr)
+ if err := Check(eventID, tc.difficulty); err != tc.wantErr {
+ t.Errorf("%d: Check(%q, %d) returned %v; want err: %v", i, eventID, tc.difficulty, err, tc.wantErr)
}
}
}
```
this part doesn't make much sense to me:
```patch
+ if Difficulty(eventID) > targetDifficulty {
+ return ErrDifficultyTooHigh
+ }
```
you'll always want to accept pow difficulty higher than target, except when **committed** is lower, so in pseudo-code:
```js
if (committedToDifficulty < targetDifficulty) {
fail
}
if (eventDifficulty < targetDifficulty) {
fail
}
```
the function i wrote in https://git.qcode.ch/nostr/nostrweb/pulls/55#issuecomment-565 should work, no?
> you'll always want to accept pow difficulty higher than target, except when committed is lower, so in pseudo-code:
not according to nip-13, see my other comment about the lucky protection
- https://git.qcode.ch/nostr/nostrweb/pulls/55#issuecomment-570
- and here the nip-13 https://github.com/nostr-protocol/nips/blob/master/13.md#mining
right. the lucky protection is about difficulty commitment, not the actual event ID pow. so i don't understand if Difficulty(eventID) > targetDifficulty { return err-too-high } - you'll want to compare that to commitment, not the actual event difficulty.
right. the lucky protection is about difficulty commitment, not the actual event ID pow. so i don't understand `if Difficulty(eventID) > targetDifficulty { return err-too-high }` - you'll want to compare that to **commitment**, not the actual event difficulty.
I expected that the difficulty arg passed to the Check function comes from the nonce tag where target difficultly is specified.
anyway I just tried to modify the go-nostr, but kind of gave up.
I expected that the difficulty arg passed to the Check function comes from the nonce tag where target difficultly is specified.
anyway I just tried to modify the go-nostr, but kind of gave up.
I expected that the difficulty arg passed to the Check function comes from the nonce tag where target difficultly is specified.
not really. look, you as a user set a minimal difficulty at which you'll accept events. let's call it acceptedDifficulty. then you verify pow for each event like so:
compare commitment from nonce against the acceptedDifficulty.
compare event pow (event ID) again with the acceptedDifficulty.
you don't really need to compare commitment and event pow between each other.
> I expected that the difficulty arg passed to the Check function comes from the nonce tag where target difficultly is specified.
not really. look, you as a user set a minimal difficulty at which you'll accept events. let's call it `acceptedDifficulty`. then you verify pow for each event like so:
1. compare **commitment** from nonce against the `acceptedDifficulty`.
2. compare event pow (**event ID**) again with the `acceptedDifficulty`.
you don't really need to compare commitment and event pow between each other.
look, you as a user set a minimal difficulty at which you'll accept events. let's call it acceptedDifficulty. then you verify pow for each event like so:
this makes sense, but I read the nip differently that a client may or may not accept an event if the commitment and event pow do not match. so some clients may reject if the actual difficulty is higher than target difficulty.
> look, you as a user set a minimal difficulty at which you'll accept events. let's call it acceptedDifficulty. then you verify pow for each event like so:
this makes sense, but I read the nip differently that a client may or may not accept an event if the commitment and event pow do not match. so some clients may reject if the actual difficulty is higher than target difficulty.
https://github.com/nostr-protocol/nips/blob/master/13.md#mining
but I am thinking in client-world, please ignore my go-nostr. I'll drop and resolve this.
maybe the go-nostr library should have an option to opt in into lucky prevention, but that is not part of this PR. :)
but I am thinking in client-world, please ignore my go-nostr. I'll drop and resolve this.
maybe the go-nostr library should have an option to opt in into lucky prevention, but that is not part of this PR. :)
@x1ddos I am not sure what to do in case of timeout, and also how long to wait.
silently dismiss is not great UX.
trying again with lower difficulty seems wrong somehow.
I guess it should show an error to the user maybe with a retry button, but that error can pop up from various places, i.e. posting, upvoting, replying etc. WDYT?
@x1ddos I am not sure what to do in case of timeout, and also how long to wait.
silently dismiss is not great UX.
trying again with lower difficulty seems wrong somehow.
I guess it should show an error to the user maybe with a retry button, but that error can pop up from various places, i.e. posting, upvoting, replying etc. WDYT?
I guess it should show an error to the user maybe with a retry button, but that error can pop up from various places, i.e. posting, upvoting, replying etc. WDYT?
yeah, i too would expect some kind of an error. a retry button would be definitely very nice 👍
> I guess it should show an error to the user maybe with a retry button, but that error can pop up from various places, i.e. posting, upvoting, replying etc. WDYT?
yeah, i too would expect some kind of an error. a retry button would be definitely very nice 👍
cleanup, and updated validatePow, but this commit does not yet
have acceptedDifficulty, it only checks if the pow is as desired.
next commit should also add timeout and a userfacing error.
adding settings to change difficulty target and timeout, so users
can change or disable pow. also added some explanation and link
to nip-13.
setting arbitrary low default to 16 zero target difficulty and
5 seconds timeout.
I'm planing to add 1 commit to give visual feedback when posting/reacting, maybe with the option to cancel mining.
also text of the settings and timeout error are very simple and could be better.
I'm planing to add 1 commit to give visual feedback when posting/reacting, maybe with the option to cancel mining.
also text of the settings and timeout error are very simple and could be better.
maybe just parseInt(localStorage.getItem('difficutly_target'))? feels a bit overkill to feed a number to a json parser. same for timeout.
also, how about renaming it to something with a word "mine", like mine_target? otherwise, i see it a bit confusing when thinking about validating others' events where an "accepted" difficulty can be different from this client's mining difficulty.
maybe just `parseInt(localStorage.getItem('difficutly_target'))`? feels a bit overkill to feed a number to a json parser. same for timeout.
also, how about renaming it to something with a word "mine", like `mine_target`? otherwise, i see it a bit confusing when thinking about validating others' events where an "accepted" difficulty can be different from this client's mining difficulty.
JSON.parse just deals with null and "0" in the best way (both valid JSON), but I guess it is a bit overkill 😥. what is convenient is: localStorage returns null for missing keys and JSON.parse just returns null as well. then it can just fallback with "nullish coalescing operator (??)", which I find cool 🤓.
`JSON.parse` just deals with `null` and `"0"` in the best way (both valid JSON), but I guess it is a bit overkill 😥. what is convenient is: localStorage returns `null` for missing keys and JSON.parse just returns `null` as well. then it can just fallback with "nullish coalescing operator (??)", which I find cool 🤓.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
compared to `parseInt(null)` returns `NaN` and `Number(null)` returns `0` which both are bit a pain to try to add a fallback.
also `parseInt` seems wrong, i.e. if the value for some reason would be "2e1" it would parse it into "2". `Number` would correctly convert to "20".
https://stackoverflow.com/questions/4090518/what-is-the-difference-between-parseint-and-number
/**
* validate proof-of-work of a nostr event per nip-13.
* the validation always requires difficulty commitment in the nonce tag.
* a difficulty param value below 1 always returns true without actual validation.
*
* @param {EventObj} evt event to validate
* @param {number} difficulty target proof-of-work difficulty
*/functionvalidatePow(evt,difficulty)
i would add a second parameter, `difficulty`:
```js
/**
* validate proof-of-work of a nostr event per nip-13.
* the validation always requires difficulty commitment in the nonce tag.
* a difficulty param value below 1 always returns true without actual validation.
*
* @param {EventObj} evt event to validate
* @param {number} difficulty target proof-of-work difficulty
*/
function validatePow(evt, difficulty)
```
this is still incomplete validation. it would return true for any difficutly as high as the commitment, but you'll want the validation to fail if the difficulty is lower than some expected value.
here's how i see this working correctly. note the second parameter:
/**
* validate proof-of-work of a nostr event per nip-13.
* the validation always requires difficulty commitment in the nonce tag.
* if targetDifficulty value is below 1, validatePow always returns true without
* actual validation.
*
* @param {EventObj} evt event to validate
* @param {number} targetDifficulty target proof-of-work difficulty
*/functionvalidatePow(evt,targetDifficulty){// skip validation for a zero target difficulty
if(!targetDifficulty||targetDifficulty<1){returntrue;}// require nonce tag and the commitment
consttag=evt.tags.find(tag=>tag[0]==='nonce');if(!tag){returnfalse;}const[,,commitment]=tag;if(!commitment||commitment<targetDifficulty){returnfalse;}// finally, check the event pow difficulty
returnzeroLeadingBitsCount(evt.id)>=targetDifficulty;}
you see, there are two importan checks against a target difficulty: first the commitment, then the event pow. but not between each other.
this is still incomplete validation. it would return true for any difficutly as high as the commitment, but you'll want the validation to fail if the difficulty is lower than some expected value.
here's how i see this working correctly. note the second parameter:
```js
/**
* validate proof-of-work of a nostr event per nip-13.
* the validation always requires difficulty commitment in the nonce tag.
* if targetDifficulty value is below 1, validatePow always returns true without
* actual validation.
*
* @param {EventObj} evt event to validate
* @param {number} targetDifficulty target proof-of-work difficulty
*/
function validatePow(evt, targetDifficulty) {
// skip validation for a zero target difficulty
if (!targetDifficulty || targetDifficulty < 1) {
return true;
}
// require nonce tag and the commitment
const tag = evt.tags.find(tag => tag[0] === 'nonce');
if (!tag) {
return false;
}
const [, , commitment] = tag;
if (!commitment || commitment < targetDifficulty) {
return false;
}
// finally, check the event pow difficulty
return zeroLeadingBitsCount(evt.id) >= targetDifficulty;
}
```
you see, there are two importan checks against a target difficulty: first the commitment, then the event pow. but not between each other.
oh, i forgot: isn't the commitment in the nonce tag a string? then need to parse it into a number:
```js
const commitment = parseInt(tag[2]);
if (!commitment || commitment < targetDifficulty) {
return false;
}
```
this is still incomplete validation. it would return true for any difficutly as high as the commitment, but you'll want the validation to fail if the difficulty is lower than some expected value.
I forgot to check validatePow and will fix zero difficulty bug.
but I was aware that at this point it doesn't require a specific target. so far the only objective was that there is nonce at all. the only change client does differently, in this PR, is conditional noxy involvement.
I assumed requiredTargetDifficulty should come in a later step as it's not clear what to do with it. but if you want it now please propose:
what should it do, keep only-noxy-invoking or something else i.e. drop events?
what targetDifficulty would you propose?
should it be configurable in the client?
prob there is more questions, alternative we keep objective to only noxy-invoking for now and get this out.
> this is still incomplete validation. it would return true for any difficutly as high as the commitment, but you'll want the validation to fail if the difficulty is lower than some expected value.
I forgot to check validatePow and will fix zero difficulty bug.
but I was aware that at this point it doesn't require a specific target. so far the only objective was that there is nonce at all. the only change client does differently, in this PR, is conditional noxy involvement.
I assumed requiredTargetDifficulty should come in a later step as it's not clear what to do with it. but if you want it now please propose:
- what should it do, keep only-noxy-invoking or something else i.e. drop events?
- what targetDifficulty would you propose?
- should it be configurable in the client?
prob there is more questions, alternative we keep objective to only noxy-invoking for now and get this out.
ah then i guess i got confused because the title says:
only invoke noxy in events with valid work
but the way it checks pow at the moment cannot say anything about "valid work". then rename the commit title? :)
another confusion for me was that the client now sends pow'ed events to a relay but expects essential no proof when receiving events. feels kind of unfair. i don't mind either way, as long as the changes are clear so i get less confused in the future!
re: what to do. those are easy questions in my head:
what should it do, keep only-noxy-invoking or something else i.e. drop events?
of course, it should drop events below a requiredTargetDifficulty. it's the whole point of nip-13, right?
what targetDifficulty would you propose?
i'd initially set it to the same as what the client mines when posting events. basically, expect the same difficulty both ways.
should it be configurable in the client?
definitely. there are many scenarios where receiving target difficuly can be different from sending. for example, slow client like an iot device. in the
future, i would also expect to be able to set separate difficulty for each relay. for example, i'd set a higher pow for a relay with many "spammy" users and lower or zero for an expensive relay. but can do this after, in a separate commit.
keep objective to only noxy-invoking for now and get this out.
yes, ship it!
ah then i guess i got confused because the title says:
> only invoke noxy in events with valid work
but the way it checks pow at the moment cannot say anything about "valid work". then rename the commit title? :)
another confusion for me was that the client now sends pow'ed events to a relay but expects essential no proof when receiving events. feels kind of unfair. i don't mind either way, as long as the changes are clear so i get less confused in the future!
re: what to do. those are easy questions in my head:
> what should it do, keep only-noxy-invoking or something else i.e. drop events?
of course, it should drop events below a requiredTargetDifficulty. it's the whole point of nip-13, right?
> what targetDifficulty would you propose?
i'd initially set it to the same as what the client mines when posting events. basically, expect the same difficulty both ways.
> should it be configurable in the client?
definitely. there are many scenarios where receiving target difficuly can be different from sending. for example, slow client like an iot device. in the
future, i would also expect to be able to set separate difficulty for each relay. for example, i'd set a higher pow for a relay with many "spammy" users and lower or zero for an expensive relay. but can do this after, in a separate commit.
> keep objective to only noxy-invoking for now and get this out.
yes, ship it!
but the way it checks pow at the moment cannot say anything about "valid work". then rename the commit title? :)
yes I didn't push as I wanted to clarify if it should already drop events in the first step. now it should check for valid work although it's already valid if it has 1 zero :)
> I forgot to check validatePow and will fix zero difficulty bug.
It now checks that there is a greater than 0 difficulty commitment and that the string is a number. made a test with not a number commitment here:
https://nostr.ch/6b0c020d91a0148ee19550f30a9362429ccb2f87e789ded7edea4ca7fd886213
> but the way it checks pow at the moment cannot say anything about "valid work". then rename the commit title? :)
yes I didn't push as I wanted to clarify if it should already drop events in the first step. now it should check for valid work although it's already valid if it has 1 zero :)
another confusion for me was that the client now sends pow'ed events to a relay but expects essential no proof when receiving events.
yes a bit unfair. but honest clients that put in work may see noxy preview again one day (if we enable it, i.e. for events with difficulty 16).
i don't mind either way, as long as the changes are clear so i get less confused in the future!
I'll update the commit message one more time, in some commit it still mentions target difficulty, but I'll change it to mining commitment.
> another confusion for me was that the client now sends pow'ed events to a relay but expects essential no proof when receiving events.
yes a bit unfair. but honest clients that put in work may see noxy preview again one day (if we enable it, i.e. for events with difficulty 16).
> i don't mind either way, as long as the changes are clear so i get less confused in the future!
I'll update the commit message one more time, in some commit it still mentions target difficulty, but I'll change it to mining commitment.
of course, it should drop events below a requiredTargetDifficulty. it's the whole point of nip-13, right?
this would have the effect that on nostr.ch only events from nostr.ch would be visible as no other clients implemented nip-13.
> of course, it should drop events below a requiredTargetDifficulty. it's the whole point of nip-13, right?
this would have the effect that on nostr.ch only events from nostr.ch would be visible as no other clients implemented nip-13.
this would have the effect that on nostr.ch only events from nostr.ch would be visible as no other clients implemented nip-13.
for public feed, yes. it's one of the main points to reduce spam - the second bullet in n.2 in #51
my hope is other clients will start doing it too. then public feed on nostrweb will start showing more by default.
anyway, that's for default public feed only. users can then tweak settings and even set pow check/mining to zero, to disable, if they want to see spam. i guess one could call it a spam gauge control? :D
> this would have the effect that on nostr.ch only events from nostr.ch would be visible as no other clients implemented nip-13.
for public feed, yes. it's one of the main points to reduce spam - the second bullet in n.2 in https://git.qcode.ch/nostr/nostrweb/issues/51#issuecomment-519
my hope is other clients will start doing it too. then public feed on nostrweb will start showing more by default.
anyway, that's for default public feed only. users can then tweak settings and even set pow check/mining to zero, to disable, if they want to see spam. i guess one could call it a spam gauge control? :D
x1ddos
added pow to text notes, reactions and metadata events.
noxy is now only used if event has valid work proof.
updates #51
good start! i would also mention that nostrweb now does pow in the readme and about page so users know what's going on.
also, made an issue in nostr-tools repo: https://github.com/fiatjaf/nostr-tools/issues/63
@ -40,6 +41,8 @@ let pubkey = localStorage.getItem('pub_key') || (() => {return pubkey;})();const difficulty = 16;would add a comment why 16, even if it's arbitrary:
another idea for the future: make it adjustable? i'd imagine a slider in profile settings.
done, using input type number for now
@ -767,2 +770,3 @@content: '+',tags: [['e', eventId, relay, 'reply']],tags: [['nonce', '0', `${difficulty}`],i would add this tag in the
powEventfunction. otherwise, you need to insert it everywhere. it's also easier to read and verify correctness when pow-related things are in the single function imho:@ -964,0 +980,4 @@return evt.id.substring(0, difficulty2 / 4) === '00'.repeat(difficulty2 / 8);}function powEvent(newEvent, difficulty) {is it possible to make it async? i'm hoping when invoked as
await powEvent(...)it UI thread in browser's javascript won't be blocked.also, i think it needs a
timeoutarg, to run at most that long. otherwise, it could run indefinitely and go crazy with the cpu fan :)another idea is to run pow in a web worker. i imagine something like this:
I played around with the worker a bit, unfortunately it seems passing a worker by URL.createObjectURL doesn't include
getEventHashbynostr-tools, this is why I added a worker.js file.A bigger issue is that
getEventHashneeds the private-key, which would be hidden from JavaScript withwindow.nostr.wait, i'm confused. why does
getEventHashneed a private key? here it takes only theEvent: https://github.com/nbd-wtf/nostr-tools/blob/4b36848b/event.ts#L56maybe we're using an older version?
the
Eventshould indeed contain the pubkey, for hashing, but i would expect the caller to set it before passing the event on to the worker.indeed, now I am confused why did I add it. thanks for pointing this out. will remove privatekey.
@ -964,0 +986,4 @@// const until = Date.now() + 15000;console.time('pow');while (true/*Date.now() < until*/) {newEvent.tags[0][1] = `${n++}`;i'm afraid this results in exponent format for large
n:but we always want regular format based 10, right?
changed to using BigInt
@ -964,0 +987,4 @@console.time('pow');while (true/*Date.now() < until*/) {newEvent.tags[0][1] = `${n++}`;const id = getEventHash(newEvent, privatekey);before hashing the event, i believe you'll want to update the
created_attimestamp. from nip-13:done
@ -964,0 +988,4 @@while (true/*Date.now() < until*/) {newEvent.tags[0][1] = `${n++}`;const id = getEventHash(newEvent, privatekey);if (id.substring(0, chars * 2) === '00'.repeat(chars)) {this unfortunately won't count the exact number of bits in all cases. taking the event id from nip-13 example, it's binary representation is:
or for better readability:
so, we have
8 + 8 + 8 + 8 + 4 = 36leading 0 bits but the current impl will count only8*4 = 32.i saw you
difficulty / 8to make it round but i think it's relatively easy to do it at the bit level. played around a bit. here's a version i came up with that counts exact number:and here's another version. it looks simpler but i don't know which one is faster. it should be possible to benchmark cpu and memory performance, and pick the best one?
first function is faster. It varies with how many 0's, tested a bit with https://jsbench.me/
👍 same in go. i added a similar function to go-nostr in https://github.com/nbd-wtf/go-nostr/pull/27
thanks for the feedback. sorry I got sick a few days ago, today is the first time I am up again. will still take a few more days.
@ -964,0 +973,4 @@return false;}const [, , difficulty2] = tag;if (difficulty2 < 16) {there's a
const difficulty = 16;elsewhere in main.js. i think you'll want to compare with it here?maybe a better way to name the const is something like
const acceptedDifficulty = 16so that it reads more clear here:done
@ -964,0 +976,4 @@if (difficulty2 < 16) {return false;}return evt.id.substring(0, difficulty2 / 4) === '00'.repeat(difficulty2 / 8);looks like that return assumes a difficulty is always divisible by 4 or 8 but it isn't always the case. could be 3, 7, 9, 11... plus, this is only commitment which can be fake.
i believe you'll want to use
zeroLeadingBitsCounthere too. for example:@ -0,0 +18,4 @@// console.timeEnd('pow');// return false;// }if (now !== event.created_at) {i think it shoudn't matter? what if you always
event.created_at = now()on each iteration?@ -0,0 +24,4 @@}event.tags[0][1] = (++n).toString();const id = getEventHash(event, privatekey);if (zeroLeadingBitsCount(id) === difficulty) {one can get luckly and produce a higher difficulty. so, the check should be an inequality:
The nip-13 has difficulty commitment target / lucky protection :)
https://github.com/nostr-protocol/nips/blob/master/13.md#mining
I tried modifying
go-nostrand added ErrDifficultyTooHigh, but somehow the tests always pass withgo testorgo test -run nip13_test.go(even if I modify the test so it should fail), doing something wrong 🤔this part doesn't make much sense to me:
you'll always want to accept pow difficulty higher than target, except when committed is lower, so in pseudo-code:
the function i wrote in #55 should work, no?
not according to nip-13, see my other comment about the lucky protection
right. the lucky protection is about difficulty commitment, not the actual event ID pow. so i don't understand
if Difficulty(eventID) > targetDifficulty { return err-too-high }- you'll want to compare that to commitment, not the actual event difficulty.I expected that the difficulty arg passed to the Check function comes from the nonce tag where target difficultly is specified.
anyway I just tried to modify the go-nostr, but kind of gave up.
not really. look, you as a user set a minimal difficulty at which you'll accept events. let's call it
acceptedDifficulty. then you verify pow for each event like so:acceptedDifficulty.acceptedDifficulty.you don't really need to compare commitment and event pow between each other.
this makes sense, but I read the nip differently that a client may or may not accept an event if the commitment and event pow do not match. so some clients may reject if the actual difficulty is higher than target difficulty.
https://github.com/nostr-protocol/nips/blob/master/13.md#mining
but I am thinking in client-world, please ignore my go-nostr. I'll drop and resolve this.
maybe the go-nostr library should have an option to opt in into lucky prevention, but that is not part of this PR. :)
@x1ddos I am not sure what to do in case of timeout, and also how long to wait.
silently dismiss is not great UX.
trying again with lower difficulty seems wrong somehow.
I guess it should show an error to the user maybe with a retry button, but that error can pop up from various places, i.e. posting, upvoting, replying etc. WDYT?
yeah, i too would expect some kind of an error. a retry button would be definitely very nice 👍
added a user facing error, planing to need to do some more testing and squash some of the commits prob tomorrow. need to rest a bit now zzzz
41efd4626fto67920b1cb62 years ago67920b1cb6todaf57d21602 years agodaf57d2160tof834268a672 years agof834268a67tob231c4cb612 years agob231c4cb61to5b538246062 years ago5b53824606to85dd6c11d82 years ago3929e572c3to67e175cd892 years ago67e175cd89to18c0dc62a62 years agoI'm planing to add 1 commit to give visual feedback when posting/reacting, maybe with the option to cancel mining.
also text of the settings and timeout error are very simple and could be better.
18c0dc62a6to06a4ffb3402 years ago06a4ffb340toa9be17ad422 years agoPTAL
very nice! there's one important bug to resolve here - see comments around
validatePow.@ -86,0 +99,4 @@<label class="number" for="miningTimeout"><span>mining timeout<br><small>abord trying to find a proof if timeout (in seconds) exceeds. use 0 to mine without a time limit.</small>abord -> abort typo
@ -749,2 +751,4 @@}// arbitrary difficulty default, still experimenting.let difficulty = JSON.parse(localStorage.getItem('difficutly_target')) ?? 16;maybe just
parseInt(localStorage.getItem('difficutly_target'))? feels a bit overkill to feed a number to a json parser. same for timeout.also, how about renaming it to something with a word "mine", like
mine_target? otherwise, i see it a bit confusing when thinking about validating others' events where an "accepted" difficulty can be different from this client's mining difficulty.JSON.parsejust deals withnulland"0"in the best way (both valid JSON), but I guess it is a bit overkill 😥. what is convenient is: localStorage returnsnullfor missing keys and JSON.parse just returnsnullas well. then it can just fallback with "nullish coalescing operator (??)", which I find cool 🤓.https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
compared to
parseInt(null)returnsNaNandNumber(null)returns0which both are bit a pain to try to add a fallback.also
parseIntseems wrong, i.e. if the value for some reason would be "2e1" it would parse it into "2".Numberwould correctly convert to "20".https://stackoverflow.com/questions/4090518/what-is-the-difference-between-parseint-and-number
@ -960,0 +1038,4 @@* @param {EventObj} evt to validate* @returns boolean*/function validatePow(evt) {i would add a second parameter,
difficulty:will add this in a later change, together with when the client actually drops events based on this.
@ -960,0 +1044,4 @@return false;}const [, , difficultyCommitment] = tag;return zeroLeadingBitsCount(evt.id) >= difficultyCommitment;this is still incomplete validation. it would return true for any difficutly as high as the commitment, but you'll want the validation to fail if the difficulty is lower than some expected value.
here's how i see this working correctly. note the second parameter:
you see, there are two importan checks against a target difficulty: first the commitment, then the event pow. but not between each other.
oh, i forgot: isn't the commitment in the nonce tag a string? then need to parse it into a number:
I forgot to check validatePow and will fix zero difficulty bug.
but I was aware that at this point it doesn't require a specific target. so far the only objective was that there is nonce at all. the only change client does differently, in this PR, is conditional noxy involvement.
I assumed requiredTargetDifficulty should come in a later step as it's not clear what to do with it. but if you want it now please propose:
what should it do, keep only-noxy-invoking or something else i.e. drop events?
what targetDifficulty would you propose?
should it be configurable in the client?
prob there is more questions, alternative we keep objective to only noxy-invoking for now and get this out.
ah then i guess i got confused because the title says:
but the way it checks pow at the moment cannot say anything about "valid work". then rename the commit title? :)
another confusion for me was that the client now sends pow'ed events to a relay but expects essential no proof when receiving events. feels kind of unfair. i don't mind either way, as long as the changes are clear so i get less confused in the future!
re: what to do. those are easy questions in my head:
of course, it should drop events below a requiredTargetDifficulty. it's the whole point of nip-13, right?
i'd initially set it to the same as what the client mines when posting events. basically, expect the same difficulty both ways.
definitely. there are many scenarios where receiving target difficuly can be different from sending. for example, slow client like an iot device. in the
future, i would also expect to be able to set separate difficulty for each relay. for example, i'd set a higher pow for a relay with many "spammy" users and lower or zero for an expensive relay. but can do this after, in a separate commit.
yes, ship it!
It now checks that there is a greater than 0 difficulty commitment and that the string is a number. made a test with not a number commitment here:
https://nostr.ch/6b0c020d91a0148ee19550f30a9362429ccb2f87e789ded7edea4ca7fd886213
yes I didn't push as I wanted to clarify if it should already drop events in the first step. now it should check for valid work although it's already valid if it has 1 zero :)
yes a bit unfair. but honest clients that put in work may see noxy preview again one day (if we enable it, i.e. for events with difficulty 16).
I'll update the commit message one more time, in some commit it still mentions target difficulty, but I'll change it to mining commitment.
this would have the effect that on nostr.ch only events from nostr.ch would be visible as no other clients implemented nip-13.
for public feed, yes. it's one of the main points to reduce spam - the second bullet in n.2 in #51
my hope is other clients will start doing it too. then public feed on nostrweb will start showing more by default.
anyway, that's for default public feed only. users can then tweak settings and even set pow check/mining to zero, to disable, if they want to see spam. i guess one could call it a spam gauge control? :D
lgtm. good start! can't wait 'till the cycle is closed and receiving side also validates pow.
a9be17ad42to825d4b25b42 years ago825d4b25b4toe1a1381a872 years agoe1a1381a87to37f0a07cf32 years ago37f0a07cf3into master 2 years agoReviewers
37f0a07cf3.Step 1:
From your project repository, check out a new branch and test the changes.Step 2:
Merge the changes and update on Gitea.