💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565803681)
> if this is smaller than other, return greater?
Yes, because "Smaller num_picked ... mean greater priority". I think it is natural to have `this` on the left hand side and `other` on the right of the comparison.
These store a point in time, not number of seconds (interval length). To me it looks that `NodeClock::time_point` is the correct type for that. "Point in time" might be implemented as number of seconds since epoch, but this is internal to the std library.
I prefer to separate i
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565803681)
> if this is smaller than other, return greater?
Yes, because "Smaller num_picked ... mean greater priority". I think it is natural to have `this` on the left hand side and `other` on the right of the comparison.
These store a point in time, not number of seconds (interval length). To me it looks that `NodeClock::time_point` is the correct type for that. "Point in time" might be implemented as number of seconds since epoch, but this is internal to the std library.
I prefer to separate i
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565815751)
Both `*foo` and `foo.value()` are valid. I find the latter more readable and prefer to keep it. Same with `if (foo)` vs `if (foo.has_value())`.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565815751)
Both `*foo` and `foo.value()` are valid. I find the latter more readable and prefer to keep it. Same with `if (foo)` vs `if (foo.has_value())`.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565823670)
Right! Done.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565823670)
Right! Done.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565840524)
Might as well pass the number to the constructor, but I think that is ungrounded. Sometimes it will benefit because it will do less expansions, but sometimes it will allocate unnecessary more. For this case in particular where we don't store many elements and is not called frequently (aka performance is not critical) I find the prevailing factor to be to have less code.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565840524)
Might as well pass the number to the constructor, but I think that is ungrounded. Sometimes it will benefit because it will do less expansions, but sometimes it will allocate unnecessary more. For this case in particular where we don't store many elements and is not called frequently (aka performance is not critical) I find the prevailing factor to be to have less code.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565842397)
Both seem to be valid. I prefer the current one.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565842397)
Both seem to be valid. I prefer the current one.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565845046)
I think it is better to have it as a comment.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565845046)
I think it is better to have it as a comment.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565856458)
Like in separate commits?
I have this in my `~/.gitconfig`:
```gitconfig
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565856458)
Like in separate commits?
I have this in my `~/.gitconfig`:
```gitconfig
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565864221)
one is moved up, the other is down. We need to check that the code in-between doesn't mutate their dependencies - it's not about diffs, those are trivial to check, but making sure the state of the world doesn't change is the tough part.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565864221)
one is moved up, the other is down. We need to check that the code in-between doesn't mutate their dependencies - it's not about diffs, those are trivial to check, but making sure the state of the world doesn't change is the tough part.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565870274)
> This is redundant
Yes, if the transaction is valid. This code is fed transactions from outside and does not do own validation, so it cannot be sure that the caller has validated. I slightly incline towards keeping it as it is because it is harmless.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565870274)
> This is redundant
Yes, if the transaction is valid. This code is fed transactions from outside and does not do own validation, so it cannot be sure that the caller has validated. I slightly incline towards keeping it as it is because it is harmless.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3582423083)
`12dfb1cb7c...50c8320f20`: take some suggestions and minor tweaks
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3582423083)
`12dfb1cb7c...50c8320f20`: take some suggestions and minor tweaks
💬 hebasto commented on pull request "ci: Run GUI unit tests in cross-Windows task":
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3582434851)
> Looks like the output is missing...
The same happens for the native build: https://github.com/hebasto/bitcoin/actions/runs/19707163063/job/56457637457.
It seems to be a peculiarity of the GHA Windows image.
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3582434851)
> Looks like the output is missing...
The same happens for the native build: https://github.com/hebasto/bitcoin/actions/runs/19707163063/job/56457637457.
It seems to be a peculiarity of the GHA Windows image.
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565904681)
> Yes, if the transaction is valid.
Interesting. How can an invalid tx have different txids but same wtxids?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565904681)
> Yes, if the transaction is valid.
Interesting. How can an invalid tx have different txids but same wtxids?
🤔 hebasto reviewed a pull request: "depends: update freetype and document remaining `bitcoin-qt` runtime libs"
(https://github.com/bitcoin/bitcoin/pull/33952#pullrequestreview-3512059986)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/33952#pullrequestreview-3512059986)
Concept ACK.
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565911693)
No idea, but this way of keeping both we don't have to find out :)
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565911693)
No idea, but this way of keeping both we don't have to find out :)
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565916767)
Hmm, it cannot, I was confused. Will drop the txid comparison.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565916767)
Hmm, it cannot, I was confused. Will drop the txid comparison.
🤔 hebasto reviewed a pull request: "guix: reduce allowed exported symbols"
(https://github.com/bitcoin/bitcoin/pull/33950#pullrequestreview-3512084049)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/33950#pullrequestreview-3512084049)
Concept ACK.
👍 laanwj approved a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3512087837)
re-ACK 1b810390fe51b79c7063966c9722c01cb8a4f7bf
Since last review:
```
git range-diff 9b1a7c3e8dd78d97fbf47c2d056d043b05969176..89bbbbd257063118e6968c409e52632835b76ce8 17072f70051dc086ef57880cd14e102ed346c350..1b810390fe51b79c7063966c9722c01cb8a4f7bf
```
1. `test: Rename k1/k2 to k0/k1 in SipHash consistency tests:` - Reordering in testcase, no functional change
2. `refactor: Extract SipHash C0-C3 constants to class scope` - No changes
3. `optimization: Introduce PresaltedSipHasher for repe
...
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3512087837)
re-ACK 1b810390fe51b79c7063966c9722c01cb8a4f7bf
Since last review:
```
git range-diff 9b1a7c3e8dd78d97fbf47c2d056d043b05969176..89bbbbd257063118e6968c409e52632835b76ce8 17072f70051dc086ef57880cd14e102ed346c350..1b810390fe51b79c7063966c9722c01cb8a4f7bf
```
1. `test: Rename k1/k2 to k0/k1 in SipHash consistency tests:` - Reordering in testcase, no functional change
2. `refactor: Extract SipHash C0-C3 constants to class scope` - No changes
3. `optimization: Introduce PresaltedSipHasher for repe
...
💬 laanwj commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3582538390)
Nice. Concept ACK. Will do riscv64 guix build.
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3582538390)
Nice. Concept ACK. Will do riscv64 guix build.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2565992040)
yeah, sry, I had found it already. I realized that it essentially just concerns text being prepended, so it is mostly whoever "tells the story" on this API. I will make the changes, or have made; need to (re)check.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2565992040)
yeah, sry, I had found it already. I realized that it essentially just concerns text being prepended, so it is mostly whoever "tells the story" on this API. I will make the changes, or have made; need to (re)check.
💬 gmaxwell commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2566047719)
Might be useful to specifically include something that describes it in the literature terms, like:
"Producing an optimal linearization of a cluster is an instance of the parametric maximal weight closure problem as found in the field of mineral extraction for open pit mining."
-- just to give future researchers a hook to the literature that contributors here initially lacked.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2566047719)
Might be useful to specifically include something that describes it in the literature terms, like:
"Producing an optimal linearization of a cluster is an instance of the parametric maximal weight closure problem as found in the field of mineral extraction for open pit mining."
-- just to give future researchers a hook to the literature that contributors here initially lacked.