💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1122983669)
This input produces an `outpoints` vector with 2 of the same outpoint, which is what causes it to crash on the line `assert(bump_fees.size() == outpoints.size()`.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1122983669)
This input produces an `outpoints` vector with 2 of the same outpoint, which is what causes it to crash on the line `assert(bump_fees.size() == outpoints.size()`.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1123037832)
This is more precise than making sure the lengths are the same (and would also fix the crash):
```suggestion
for (const auto& outpoint : outpoints) {
auto it = bump_fees.find(outpoint);
assert(it != bump_fees.end());
assert(it->second >= 0);
}
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1123037832)
This is more precise than making sure the lengths are the same (and would also fix the crash):
```suggestion
for (const auto& outpoint : outpoints) {
auto it = bump_fees.find(outpoint);
assert(it != bump_fees.end());
assert(it->second >= 0);
}
```
📝 MarcoFalke opened a pull request: "Use steady clock in SeedStrengthen and FindBestImplementation"
(https://github.com/bitcoin/bitcoin/pull/27189)
There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing `SeedStrengthen`.
Fix this by using steady clock.
Do the same in `FindBestImplementation`, which shouldn't be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
(https://github.com/bitcoin/bitcoin/pull/27189)
There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing `SeedStrengthen`.
Fix this by using steady clock.
Do the same in `FindBestImplementation`, which shouldn't be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
👋 brunoerg's pull request is ready for review: "test: fix intermittent issue in `feature_bip68_sequence`"
(https://github.com/bitcoin/bitcoin/pull/27177)
(https://github.com/bitcoin/bitcoin/pull/27177)
💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1451836694)
Force-pushed changing the approach.
The logic:
- if you don't specify a `txid`, it will return the largest utxo desconsidering immature coinbase. In case the only ones available are immature ones, it will return the largest of them.
_Why?_ - We can't filter the immature coinbase ones by default because in some tests they're the only available ones.
- Added a flag `avoid_immature_coinbase` to not break some mempool tests, in that case, they need to fetch it considering the immature ones
...
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1451836694)
Force-pushed changing the approach.
The logic:
- if you don't specify a `txid`, it will return the largest utxo desconsidering immature coinbase. In case the only ones available are immature ones, it will return the largest of them.
_Why?_ - We can't filter the immature coinbase ones by default because in some tests they're the only available ones.
- Added a flag `avoid_immature_coinbase` to not break some mempool tests, in that case, they need to fetch it considering the immature ones
...
💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1123068867)
removing `Any` it throw a lint error, I believe it's because it can be any value from `filter` or `reversed`.
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1123068867)
removing `Any` it throw a lint error, I believe it's because it can be any value from `filter` or `reversed`.
💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1123069732)
Not sure. In case of specifying a `txid`, it will not have to return the largest one.
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1123069732)
Not sure. In case of specifying a `txid`, it will not have to return the largest one.
💬 TheCharlatan commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1451900443)
> The main downside of this implementation is that the entire UTXO set must be loaded in RAM (in mapCoins) before being written to disk. So running dumptxoutset will consome a lot of RAM on mainnet since the utxo set is large.
> Not sure how to improve this.
The keys in leveldb are guaranteed to be lexicographically sorted. You just need to cache the last txid and flush the txid and outpoint tuples once the next one is reached in the database iterator. I pushed a dirty proof of concept [her
...
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1451900443)
> The main downside of this implementation is that the entire UTXO set must be loaded in RAM (in mapCoins) before being written to disk. So running dumptxoutset will consome a lot of RAM on mainnet since the utxo set is large.
> Not sure how to improve this.
The keys in leveldb are guaranteed to be lexicographically sorted. You just need to cache the last txid and flush the txid and outpoint tuples once the next one is reached in the database iterator. I pushed a dirty proof of concept [her
...
💬 pablomartin4btc commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1451948439)
> The only thing i suggest changing is giving a more descriptive error on the following situation :
> ```
> $ bitcoin-cli -regtest -generate 1 -rpcwallet=tst2
> error: Error: this is not a valid cli-command argument "-rpcwallet=tst2", if that is a valid option you might try passing it before -generate
> ```
Thanks, I agree with it, updated it.
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1451948439)
> The only thing i suggest changing is giving a more descriptive error on the following situation :
> ```
> $ bitcoin-cli -regtest -generate 1 -rpcwallet=tst2
> error: Error: this is not a valid cli-command argument "-rpcwallet=tst2", if that is a valid option you might try passing it before -generate
> ```
Thanks, I agree with it, updated it.
💬 grubles commented on issue "make check errors on big endian OpenBSD 7.2":
(https://github.com/bitcoin/bitcoin/issues/26492#issuecomment-1452021590)
I imagine there are pretty major differences between SPARC64 and POWER9 so I'm not sure if I would rely on testing with SPARC64 here, but I could be wrong. My POWER9 machine is in use at the moment but I'll try to work in retesting this in the coming weeks.
(https://github.com/bitcoin/bitcoin/issues/26492#issuecomment-1452021590)
I imagine there are pretty major differences between SPARC64 and POWER9 so I'm not sure if I would rely on testing with SPARC64 here, but I could be wrong. My POWER9 machine is in use at the moment but I'll try to work in retesting this in the coming weeks.
👍 hernanmarino approved a pull request: "cli: add validation to cli side commands besides when it's used with -rpcwallet"
(https://github.com/bitcoin/bitcoin/pull/26990)
re ACK 7f5fd1e6999b955dbb0b382bbfdce90a02e1cd8e Looks good to me now
(https://github.com/bitcoin/bitcoin/pull/26990)
re ACK 7f5fd1e6999b955dbb0b382bbfdce90a02e1cd8e Looks good to me now
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123258118)
I wrote it in that way before but changed it due a previous code review suggestion (previous [diff](https://github.com/bitcoin/bitcoin/compare/4343835a23290f2cd34404821494303cc75ce86e..5246cc1098e5409e986c1f215bc88eb44cec62f8)). I'm not fan of bool args neither but, at least here, I would be ok either way.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123258118)
I wrote it in that way before but changed it due a previous code review suggestion (previous [diff](https://github.com/bitcoin/bitcoin/compare/4343835a23290f2cd34404821494303cc75ce86e..5246cc1098e5409e986c1f215bc88eb44cec62f8)). I'm not fan of bool args neither but, at least here, I would be ok either way.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123260283)
yes, I didn't remove it just to make the diff smaller. This was already there.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123260283)
yes, I didn't remove it just to make the diff smaller. This was already there.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123270988)
What are you referring with "mixed type groups"?
The `mixed` term is only used inside the `Groups` structure, and reflects a vector of `OutputGroup` that may contain positive and negative UTXOs.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123270988)
What are you referring with "mixed type groups"?
The `mixed` term is only used inside the `Groups` structure, and reflects a vector of `OutputGroup` that may contain positive and negative UTXOs.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123295722)
The "filter" term usage here in the comment is wrong. For what corresponds to this struct, the `all` term just refers to the combination of all inserted groups. Same as we do in the `CoinsResult::All` method. The filtering process, nor what is inserted (whether is filtered or not), is not responsibility of this struct.
It's just a container that keeps track of `Groups` by type and the caches the combination of all of them.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123295722)
The "filter" term usage here in the comment is wrong. For what corresponds to this struct, the `all` term just refers to the combination of all inserted groups. Same as we do in the `CoinsResult::All` method. The filtering process, nor what is inserted (whether is filtered or not), is not responsibility of this struct.
It's just a container that keeps track of `Groups` by type and the caches the combination of all of them.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123305653)
for the `FilteredOutputGroups` map. `std::map` requires a comparator or a hash function.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123305653)
for the `FilteredOutputGroups` map. `std::map` requires a comparator or a hash function.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123314932)
`GroupsByValueSign`?
I prefer the struct rather than having an ugly std::pair or a typedef:
`typedef std::pair<std::vector<OutputGroup>, std::vector<OutputGroup>> GroupsByValueSign`
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123314932)
`GroupsByValueSign`?
I prefer the struct rather than having an ugly std::pair or a typedef:
`typedef std::pair<std::vector<OutputGroup>, std::vector<OutputGroup>> GroupsByValueSign`
💬 ryanofsky commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784)
In commit "consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts`" (97292dafb528d2a097f29e4c79a08cced49e5451)
Should this use emplace_back instead of push_back?
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784)
In commit "consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts`" (97292dafb528d2a097f29e4c79a08cced49e5451)
Should this use emplace_back instead of push_back?
⚠️ da2ce7 opened an issue: "Coin Controll for Unconfirmed Outputs"
(https://github.com/bitcoin/bitcoin/issues/27190)
I would like to be able to select unconfined outputs in coin-control. For example allowing me to make a child-pays-for-parent transaction.
(https://github.com/bitcoin/bitcoin/issues/27190)
I would like to be able to select unconfined outputs in coin-control. For example allowing me to make a child-pays-for-parent transaction.
💬 MarcoFalke commented on pull request "wallet: ensure the wallet is unlocked when needed for rescanning":
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123340738)
This fails for me when running in valgrind:
```
node0 2023-02-28T19:50:00.090808Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=walletpassphrase user=__cookie__
node0 2023-02-28T19:50:08.966047Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/server.cpp:560] [RPCRunLater] [rpc] queue run of timer lockwallet(encrypted_wallet) in 1 seconds (using HTTP)
node0 2023-02-28T19:50:08.995457Z (mocktime: 2023-03-30T19:35:09Z) [ht
...
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123340738)
This fails for me when running in valgrind:
```
node0 2023-02-28T19:50:00.090808Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=walletpassphrase user=__cookie__
node0 2023-02-28T19:50:08.966047Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/server.cpp:560] [RPCRunLater] [rpc] queue run of timer lockwallet(encrypted_wallet) in 1 seconds (using HTTP)
node0 2023-02-28T19:50:08.995457Z (mocktime: 2023-03-30T19:35:09Z) [ht
...