💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2070491742)
Rebased to fix merge conflicts
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2070491742)
Rebased to fix merge conflicts
🤔 mzumsande reviewed a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913#pullrequestreview-2015537357)
IIn my opinion setting it to `ActiveTip()` is not correct because `m_best_header` was not necessarily equal to `ActiveTip()` in the first place (before `invalidateblock`) :
During IBD they differ a lot (and `invalidateblock` / `resonsiderblock` is allowed then, too!). So after `resonsiderblock` it would still be set to the wrong value. I could verify that on signet via `getblockchaininfo` shortly after downloading the headers.
I think that the correct approach would be to iterate through h
...
(https://github.com/bitcoin/bitcoin/pull/29913#pullrequestreview-2015537357)
IIn my opinion setting it to `ActiveTip()` is not correct because `m_best_header` was not necessarily equal to `ActiveTip()` in the first place (before `invalidateblock`) :
During IBD they differ a lot (and `invalidateblock` / `resonsiderblock` is allowed then, too!). So after `resonsiderblock` it would still be set to the wrong value. I could verify that on signet via `getblockchaininfo` shortly after downloading the headers.
I think that the correct approach would be to iterate through h
...
⚠️ laanwj opened an issue: "guix: SOURCE_DATE_EPOCH is already set in some environments"
(https://github.com/bitcoin/bitcoin/issues/29935)
The environment variable `SOURCE_DATE_EPOCH` allows overriding the date that will be used inside the archives for guix-built binaries. This is an intentional feature, as documented in `contrib/guix/README.md`:
> * _**SOURCE_DATE_EPOCH**_
>
> Override the reference UNIX timestamp used for bit-for-bit reproducibility,
> the variable name conforms to [standard][r12e/source-date-epoch].
>
> _(defaults to the output of `$(git log --format=%at -1)`)_
However, some environments, as a
...
(https://github.com/bitcoin/bitcoin/issues/29935)
The environment variable `SOURCE_DATE_EPOCH` allows overriding the date that will be used inside the archives for guix-built binaries. This is an intentional feature, as documented in `contrib/guix/README.md`:
> * _**SOURCE_DATE_EPOCH**_
>
> Override the reference UNIX timestamp used for bit-for-bit reproducibility,
> the variable name conforms to [standard][r12e/source-date-epoch].
>
> _(defaults to the output of `$(git log --format=%at -1)`)_
However, some environments, as a
...
💬 hebasto commented on issue "guix: SOURCE_DATE_EPOCH is already set in some environments":
(https://github.com/bitcoin/bitcoin/issues/29935#issuecomment-2070764504)
> * Rename our `SOURCE_DATE_EPOCH` to something non-standard that doesn't conflict with Nix.
As we run Guix shell in a container, it seems reasonable to rename `SOURCE_DATE_EPOCH` in the `guix-build` script, and pass it to the container using its original name:
```
--env SOURCE_DATE_EPOCH="${BITCOIN_SOURCE_DATE_EPOCH:?unable to determine value}"
```
(https://github.com/bitcoin/bitcoin/issues/29935#issuecomment-2070764504)
> * Rename our `SOURCE_DATE_EPOCH` to something non-standard that doesn't conflict with Nix.
As we run Guix shell in a container, it seems reasonable to rename `SOURCE_DATE_EPOCH` in the `guix-build` script, and pass it to the container using its original name:
```
--env SOURCE_DATE_EPOCH="${BITCOIN_SOURCE_DATE_EPOCH:?unable to determine value}"
```
📝 brunoerg opened a pull request: "fuzz: wallet: add target for `CreateTransaction`"
(https://github.com/bitcoin/bitcoin/pull/29936)
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
...
(https://github.com/bitcoin/bitcoin/pull/29936)
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
...
💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2070810257)
Just added https://github.com/bitcoin/bitcoin/pull/29936 to the list. It's a regression to #27271.
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2070810257)
Just added https://github.com/bitcoin/bitcoin/pull/29936 to the list. It's a regression to #27271.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575302127)
Good catch, fixed
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575302127)
Good catch, fixed
👍 brunoerg approved a pull request: "rpc: Reword SighashFromStr error message"
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2015715112)
utACK fa6ab0d020d0b1492203f7eb2ccb8051812de086
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2015715112)
utACK fa6ab0d020d0b1492203f7eb2ccb8051812de086
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2070869416)
For testing big endian wallets, you can use `db_dump wallet.dat | db_load -c db_lorder=4321 wallet_big_endian.dat` to make big endian databasese using BDB's tools.
However, I also realized that BDB lets us set the endianness to use for a new database, so I've added a `bdb_swap` format to `createfromdump` that will make a BDB database with the other endianness. This will flip it depending on the system that you use, so on little endian, it will make a big endian wallet, and on big endian, it w
...
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2070869416)
For testing big endian wallets, you can use `db_dump wallet.dat | db_load -c db_lorder=4321 wallet_big_endian.dat` to make big endian databasese using BDB's tools.
However, I also realized that BDB lets us set the endianness to use for a new database, so I've added a `bdb_swap` format to `createfromdump` that will make a BDB database with the other endianness. This will flip it depending on the system that you use, so on little endian, it will make a big endian wallet, and on big endian, it w
...
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575308063)
> Is this used in this pull request?
No, it's used in the followup.
> I wonder if it could make sense to return the error string to the caller, assuming this is called by RPC eventually?
`Backup` is a part of the `WalletDatabase` parent class, so changing this would also mean changing the `Backup` of existing database classes, and all of their callers. I think that's something that would be worth doing in a followup, but not this PR.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575308063)
> Is this used in this pull request?
No, it's used in the followup.
> I wonder if it could make sense to return the error string to the caller, assuming this is called by RPC eventually?
`Backup` is a part of the `WalletDatabase` parent class, so changing this would also mean changing the `Backup` of existing database classes, and all of their callers. I think that's something that would be worth doing in a followup, but not this PR.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2070973893)
cc: @maflcko
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2070973893)
cc: @maflcko
💬 ariard commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2070974638)
> Still, what are TRUCs ? Do you have documentation ?
TRUC = Topologically Restricted Until Confirmation, see https://github.com/bitcoin/bips/pull/1541
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2070974638)
> Still, what are TRUCs ? Do you have documentation ?
TRUC = Topologically Restricted Until Confirmation, see https://github.com/bitcoin/bips/pull/1541
💬 achow101 commented on issue "Release schedule for 27.0":
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-2071021674)
@levantah Contributions are certainly welcome and desired, you can take a look at any of the open issues, particularly ones tagged as "Good first issue". However, as Bitcoin Core is security software, we only want to link to our own official sources for releases. While we appreciate the gesture you did by hosting a mirror of the release, it is still something we cannot point people to, and for the safety of our users, the comment is removed. While I don't doubt that you are only trying to help,
...
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-2071021674)
@levantah Contributions are certainly welcome and desired, you can take a look at any of the open issues, particularly ones tagged as "Good first issue". However, as Bitcoin Core is security software, we only want to link to our own official sources for releases. While we appreciate the gesture you did by hosting a mirror of the release, it is still something we cannot point people to, and for the safety of our users, the comment is removed. While I don't doubt that you are only trying to help,
...
💬 achow101 commented on pull request "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type":
(https://github.com/bitcoin/bitcoin/pull/29657#issuecomment-2071028198)
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d
(https://github.com/bitcoin/bitcoin/pull/29657#issuecomment-2071028198)
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d
🚀 achow101 merged a pull request: "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type"
(https://github.com/bitcoin/bitcoin/pull/29657)
(https://github.com/bitcoin/bitcoin/pull/29657)
💬 achow101 commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2071037489)
ACK fa6c300a9926a1d35fdd0a80f59ea39769bd2596
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2071037489)
ACK fa6c300a9926a1d35fdd0a80f59ea39769bd2596
🚀 achow101 merged a pull request: "test: Fix intermittent timeout in p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/29933)
(https://github.com/bitcoin/bitcoin/pull/29933)
💬 achow101 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575420781)
In caa5242f84a6e1ef90b8a44c1206c7b5b942a00c "cli: Sanitize ports in rpcconnect and rpcport"
This will fail if you just have a regtest node running independent of the test:
```
2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
fun(*args, **kwds)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_fram
...
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575420781)
In caa5242f84a6e1ef90b8a44c1206c7b5b942a00c "cli: Sanitize ports in rpcconnect and rpcport"
This will fail if you just have a regtest node running independent of the test:
```
2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
fun(*args, **kwds)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_fram
...
👍 sdaftuar approved a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2005939457)
Code review ACK (apart from the tests, which I only skimmed). Will test...
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2005939457)
Code review ACK (apart from the tests, which I only skimmed). Will test...
💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568781077)
This is a code style nit, but I think I agree with https://github.com/bitcoin/bitcoin/pull/21062/files#r571975710 that there's not much benefit from using a `std::optional` on `m_replaced_transactions`? It just seems to lead to extra code around understanding when the field is set or not, when I think it would be simpler to say that it's just a list which is either empty or non-empty based on whether a replacement took place.
Anyway I have no objection to the changes in this commit; just wan
...
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568781077)
This is a code style nit, but I think I agree with https://github.com/bitcoin/bitcoin/pull/21062/files#r571975710 that there's not much benefit from using a `std::optional` on `m_replaced_transactions`? It just seems to lead to extra code around understanding when the field is set or not, when I think it would be simpler to say that it's just a list which is either empty or non-empty based on whether a replacement took place.
Anyway I have no objection to the changes in this commit; just wan
...