👍 maflcko approved a pull request: "rpc: dumptxoutset height parameter follow-ups (29553)"
(https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2279769878)
review ACK e6812ffec338035a1ebe949015e95ba335c94d08 💦
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e6812ffec338
...
(https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2279769878)
review ACK e6812ffec338035a1ebe949015e95ba335c94d08 💦
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e6812ffec338
...
💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743586494)
nit in e6812ffec338035a1ebe949015e95ba335c94d08: Wouldn't it be easier to just `throw` the error on creation, instead of creating a temporary symbol and check whether it is null and then `throw` it? With `TemporaryRollback` this should be possible now.
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743586494)
nit in e6812ffec338035a1ebe949015e95ba335c94d08: Wouldn't it be easier to just `throw` the error on creation, instead of creating a temporary symbol and check whether it is null and then `throw` it? With `TemporaryRollback` this should be possible now.
💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743571147)
```suggestion
// process which may not be what the user wants.
```
nit in9467de52d3befd831b16b01c332793de18cb29cc : typo
Also "Don't do this if ..." can be replaced by "Skip if ...", but either is fine.
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743571147)
```suggestion
// process which may not be what the user wants.
```
nit in9467de52d3befd831b16b01c332793de18cb29cc : typo
Also "Don't do this if ..." can be replaced by "Skip if ...", but either is fine.
💬 instagibbs commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743671441)
I feel like the ability to fetch the whole transaction (set) optionally is useful, otherwise we just have a couple hashes of the transactions, no ability to estimate how many vbytes the orphanage is using, do any analysis on the data given?
Maybe have an optional argument via txid/wtxid that fetches the full serialized transaction in hex?
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743671441)
I feel like the ability to fetch the whole transaction (set) optionally is useful, otherwise we just have a couple hashes of the transactions, no ability to estimate how many vbytes the orphanage is using, do any analysis on the data given?
Maybe have an optional argument via txid/wtxid that fetches the full serialized transaction in hex?
💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743675879)
Agreed, will refinea
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743675879)
Agreed, will refinea
💬 instagibbs commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
Follow-up PRs can probably add more interesting fields as well, such as entry time, announcer peerid, etc.
I'll try not to scope creep this PR too bad
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
Follow-up PRs can probably add more interesting fields as well, such as entry time, announcer peerid, etc.
I'll try not to scope creep this PR too bad
💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743691588)
For originator, I was thinking `fromPeer` (the NodeId). Currently each orphan tracks 1 sender, but hoping to have multiple in the future (#28031 if you're curious).
For this PR, I think it would suffice to take an approach similar to `getrawmempool` but with an integer verbosity.
verbose=0: list of txids (it's ok if there are duplicates, it should be rare anyway)
verbose=1: list of objects with txid, wtxid, size, senders (a list of NodeIds which as of now has length 1), expiration
I thin
...
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743691588)
For originator, I was thinking `fromPeer` (the NodeId). Currently each orphan tracks 1 sender, but hoping to have multiple in the future (#28031 if you're curious).
For this PR, I think it would suffice to take an approach similar to `getrawmempool` but with an integer verbosity.
verbose=0: list of txids (it's ok if there are duplicates, it should be rare anyway)
verbose=1: list of objects with txid, wtxid, size, senders (a list of NodeIds which as of now has length 1), expiration
I thin
...
💬 hebasto commented on issue "guix: Build fails for `x86_64-apple-darwin`":
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2328862764)
> Has this ever worked on your system? `guix shell: error` sounds like it is an error unrelated to Bitcoin Core?
>
> Can you use guix and guix shell normally?
Both command work just fine. For example:
```
$ guix package -i hello
$ guix shell python -- python3
```
Moreover, every other non-Darwin host is built successfully with `./contrib/guix/guix-build`.
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2328862764)
> Has this ever worked on your system? `guix shell: error` sounds like it is an error unrelated to Bitcoin Core?
>
> Can you use guix and guix shell normally?
Both command work just fine. For example:
```
$ guix package -i hello
$ guix shell python -- python3
```
Moreover, every other non-Darwin host is built successfully with `./contrib/guix/guix-build`.
💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743697939)
> ability to estimate how many vbytes the orphanage is using
A `getorphanageinfo` can be added for overall stats, perhaps after #28031 which adds size tracking.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743697939)
> ability to estimate how many vbytes the orphanage is using
A `getorphanageinfo` can be added for overall stats, perhaps after #28031 which adds size tracking.
💬 fanquake commented on issue "guix: Build fails for `x86_64-apple-darwin`":
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2328969074)
I think you'll need to debug this further. It's odd that building for all HOSTS (except one) would work, but building for `x86_64-apple-darwin` currently works fine, as far as I'm aware (just tested on two different systems).
> Both command work just fine. For example:
It's not clear that those are actually invoking the issue-causing behaviour. The error message points to `guix container`. i.e `guix shell --container`
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2328969074)
I think you'll need to debug this further. It's odd that building for all HOSTS (except one) would work, but building for `x86_64-apple-darwin` currently works fine, as far as I'm aware (just tested on two different systems).
> Both command work just fine. For example:
It's not clear that those are actually invoking the issue-causing behaviour. The error message points to `guix container`. i.e `guix shell --container`
🚀 fanquake merged a pull request: "fuzz: Rename fuzz_seed_corpus to fuzz_corpora"
(https://github.com/bitcoin/bitcoin/pull/30804)
(https://github.com/bitcoin/bitcoin/pull/30804)
💬 garlonicon commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329009795)
I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.
So, if anyone has any coins, which were received after block number 42335, then they
...
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329009795)
I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.
So, if anyone has any coins, which were received after block number 42335, then they
...
💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2329027542)
> @sdaftuar do you think you could instrument your code to gather/dump clusters that linearize optimally with say 1,000,000 iterations but not using 100,000 iterations, using the code up to commit [21b826a](https://github.com/bitcoin/bitcoin/commit/21b826af07fdfa26aaf981cc0810edd6b55b45e0) (before that commit the search algorithm is so naive that high iteration count isn't really an indication of being hard).
I used my simulation environment to run the commit you listed on last year's data, s
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2329027542)
> @sdaftuar do you think you could instrument your code to gather/dump clusters that linearize optimally with say 1,000,000 iterations but not using 100,000 iterations, using the code up to commit [21b826a](https://github.com/bitcoin/bitcoin/commit/21b826af07fdfa26aaf981cc0810edd6b55b45e0) (before that commit the search algorithm is so naive that high iteration count isn't really an indication of being hard).
I used my simulation environment to run the commit you listed on last year's data, s
...
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743693696)
style nit in 95b3aca0eb3fc49cd99b8de028a676bb04e2157f: Could have the happy-case early-return in the second line of the function, like in a prior version of this pull request.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743693696)
style nit in 95b3aca0eb3fc49cd99b8de028a676bb04e2157f: Could have the happy-case early-return in the second line of the function, like in a prior version of this pull request.
🤔 maflcko reviewed a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280021001)
Concept ACK.
I left another question in the second commit. Sorry for the incremental review.
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280021001)
Concept ACK.
I left another question in the second commit. Sorry for the incremental review.
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743729122)
question in 948bb07715f9ef1d90b91dfbea4f487f755498f5: Is there a reason why the tests are dropped? I read the commit message and I understand that the comment says "String constructors", but I think it meant to say `UintToArith256 testing from a string`, which is not a test-only function.
This is my fault, because I forgot to update the comment in commit facf629ce8ff1d1f6d254dde4e89b5018f8df60e.
I also understand that there is a `BOOST_AUTO_TEST_CASE(conversion)` test. However, I think it
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743729122)
question in 948bb07715f9ef1d90b91dfbea4f487f755498f5: Is there a reason why the tests are dropped? I read the commit message and I understand that the comment says "String constructors", but I think it meant to say `UintToArith256 testing from a string`, which is not a test-only function.
This is my fault, because I forgot to update the comment in commit facf629ce8ff1d1f6d254dde4e89b5018f8df60e.
I also understand that there is a `BOOST_AUTO_TEST_CASE(conversion)` test. However, I think it
...
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743786908)
Sorry for misunderstanding, but I think an error message of
`"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f')"` is unclear whether the error is that there are non-hex digits or whether there are missing or extraneous characters. And it is not trivial to see from looking at it.
A proposed error message of `"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f' of len $num)"` wh
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743786908)
Sorry for misunderstanding, but I think an error message of
`"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f')"` is unclear whether the error is that there are non-hex digits or whether there are missing or extraneous characters. And it is not trivial to see from looking at it.
A proposed error message of `"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f' of len $num)"` wh
...
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743805522)
I tend to agree that the length is superfluous (we're usually not *that* user friendly with our error messages) and in case of error the user would likely copy-paste again which would likely solve the error.
I'm fine with both, just don't revert anything @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743805522)
I tend to agree that the length is superfluous (we're usually not *that* user friendly with our error messages) and in case of error the user would likely copy-paste again which would likely solve the error.
I'm fine with both, just don't revert anything @stickies-v :)
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743810469)
Some of these were reverted since it seemed to us that it's testing prefixes and whitespaces primarily, see https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743810469)
Some of these were reverted since it seemed to us that it's testing prefixes and whitespaces primarily, see https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743817035)
I agree the prefix and whitespace testing is useless and the prefix and whitespace can simply be removed in the test cases, but it is not trivially clear to me that the `UintToArith256` tests can be removed.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743817035)
I agree the prefix and whitespace testing is useless and the prefix and whitespace can simply be removed in the test cases, but it is not trivially clear to me that the `UintToArith256` tests can be removed.