🤔 naumenkogs reviewed a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1850607392)
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

💬 naumenkogs commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#discussion_r1470830112)
i'm not sure we want to keep `default_to_v2`. It's kinda unused now, and I'm not sure future tests would ever find it useful.

👍 theStack approved a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1850619543)
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1916411990)
> I dont really understand how this can affect coinselection, can you expand please? Thanks

Sorry, I was completely overthinking it! I was incorrectly thinking that we took `max_tx_fee` and `tx_fee_rate` as inputs to coin selection, but what we do instead:

1. Check the if the user provided a `fee_rate` (this fee rate would have already been sanity checked against `max_fee_rate`)
2. If not, get a fee rate by doing some checks against min fees and fee estimation
3. Check `max_tx_fee` *afte
...

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1470879755)
What is the point of parsing as signed integer and then casting to unsigned? Why not use `ParseUInt32` directly?

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1916447568)
@ismaelsadeeq thanks for walking through the scenario! Reading over what you posted, if `maxtxfee` is left as the default, then the user can set whatever they want for `maxfeerate` without any issues. Depending on the types of transactions they make, they are essentially setting a new, lower `maxtxfee`.

If the user is setting both, they could create weird scenarios where the `maxtxfee` is too low compared to the `maxtxfeerate` and vice versa. But thinking it through and with your examples, I
...

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1470885445)
perhaps introduce a new error type:

```suggestion
return util::Error{TransactionErrorString(TransactionError::MAX_FEE_RATE_EXCEEDED)};
```

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1470882974)
nit: Clang-format while touching this line of code?


```cpp
static const uint32_t CURRENT_VERSION{2};

💬 ajtowns commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1916502753)
Isn't this check a little bit later than ideal? Wouldn't it make more sense to check both maxfeerate and maxburnamount as part of `signrawtransactionwith*` and `walletprocesspsbt` so that you never create a tx that can lose your money, rather than creating it and hoping that it won't find its way onto the network via some method other than rpc with default args?

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1916514936)
> If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core is generating this key, I don't see why we are writing it to disk unencrypted.

We generate the key for Stratum v2. We don't generate the key for Tor and I2P. We also don't encrypt those.

I don't see how encryption could work for a server application like a Template Provider. Putting the password in a config file adds complexity and no security
...

💬 kevkevinpal commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-1916529533)
Concept ACK [ebb842a](https://github.com/bitcoin/bitcoin/pull/29335/commits/ebb842ae4fbb20d2933e6852afd055bedeacf8c2)

💬 kevkevinpal commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1470966212)
I think we can reword this to
`Tests aborted: Insufficient space available in directory: {tmpdir}`

Since the device may have space but the `tmpdir` we are using might have insufficient space

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1470968822)
Interestingly, in https://github.com/bitcoin/bitcoin/blob/411ba32af21a56efa0a570b6aa8bf8f035410230/src/rpc/mempool.cpp#L89

`sendrawtransaction` takes a fee_rate argument, which is used to calculate a `max_tx_fee` based on the size of the transaction, and this `max_tx_fee` is passed to `BroadcastTransaction`. This means a user could typo an insane `maxfeerate` when calling `sendrawtransaction` that bypasses the default max tx fee.

Perhaps it does make more sense to have `BroadcastTransactio
...

💬 glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1916562018)
> Isn't this check a little bit later than ideal? Wouldn't it make more sense to check both maxfeerate and maxburnamount as part of `signrawtransactionwith*` and `walletprocesspsbt` so that you never create a tx that can lose your money

I think it's indeed good to have that, but isn't it still possible that transactions submitted via `submitpackage` was created somewhere else?

💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1916576350)
> It would help if you read the Stratum v2 spec on this

This PR is adding generic `Serialize/Deserialize` commands to `CKey` which allows you to read and write 32 bytes without any context. I don't think this is a good change for Bitcoin Core.

After talking through your specific usecase for SV2, I better understand *why* you want to read and write a key to disk and how you intend to provide context for the key, but I don't think it's a compelling enough reason to motivate a general change
...

💬 dergoegge commented on pull request "redeclare nChainTx to use uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1916582679)
> It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1907059722 to allow that per RPC could make sense (not necessarily in this PR).

I was thinking of a unit test, so no need for an RPC but yes some way to add assumeutxo data for tests would be needed (I assumed we already have this).

💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1471026049)
The total size of the generated testdir after running `./test_runner.py --nocleanup` is `11GB` on my system

See the list of the generated folders and their sizes on the attached file below:
`sudo du -h /tmp/test_runner_₿_🏃_20240130_125641/ > ~/tmp-test_runner_₿_🏃_20240130_125641-test-files.txt`

[tmp-test_runner_₿_🏃_20240130_125641-test-files.txt](https://gith
...

💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1471035243)
```
1.1G /tmp/test_runner_₿_🏃_20240130_125641/feature_block_289
```

So this value seems wrong either way? How can 66 MB be enough to fit 1.1GB, or even 11GB?

⚠️ Rucade opened an issue: "bitcoin core v.26 shuts down without warning - Doesnt save blocks downloaded"
(https://github.com/bitcoin/bitcoin/issues/29348)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

bitcoin core v.26 shuts down all the time unexpectedly, crashes and asks to Force Quit or wait so it does not save the blocks downloaded unless I shut it down first which is a terrible task every few minutes. I am at June 22nd 2023 for days trying to download the rest of the blocks. How do I fix this "bug"?

### Expected behaviour

to get the core loading properly I need to fix this bug or
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1916637496)
Added a change to fix the `make deploy` CI failure. It involves switching from `otool`, to `llvm-objdump`, which for our needs (listing shared lib deps) is a drop-in replacement with the right flags (`--macho` `--dylibs-used`). We could have used `llvm-otool`, but we'd have the same problem that we would have had with `llvm-libtool`, which is that some distros (Ubuntu) only ship this binary with a version-suffix, which makes it annoying to find/use. Using `llvm-objdump` avoids this issue, and it
...