💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491468929)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Note: effects of changing this definition seem to be:
- `CachedTxIsTrusted` is changed, so if a mempool conflict with a transaction is detected, the transaction will no longer be trusted, and its outputs will show up in the unconfirmed balance and no longer be spent from.
- `isUnconfirmed` is changed, so `ResubmitWalletTransactions` is changed, so if a mempool conflict with a transaction is detected, th
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491468929)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Note: effects of changing this definition seem to be:
- `CachedTxIsTrusted` is changed, so if a mempool conflict with a transaction is detected, the transaction will no longer be trusted, and its outputs will show up in the unconfirmed balance and no longer be spent from.
- `isUnconfirmed` is changed, so `ResubmitWalletTransactions` is changed, so if a mempool conflict with a transaction is detected, th
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491562385)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Seems like a minor bug here. If this returns `TxUpdate::UNCHANGED` it means RecursiveUpdateTxState will stop recursing, so the parent transaction will have its `mempool_conflicts` list updated, but its descendants will not.
In practice, this should be a minor bug because the only time `TxUpdate::UNCHANGED` is actually returned here is when the transaction is in `BlockConflicted` state, and it shouldn't b
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491562385)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
Seems like a minor bug here. If this returns `TxUpdate::UNCHANGED` it means RecursiveUpdateTxState will stop recursing, so the parent transaction will have its `mempool_conflicts` list updated, but its descendants will not.
In practice, this should be a minor bug because the only time `TxUpdate::UNCHANGED` is actually returned here is when the transaction is in `BlockConflicted` state, and it shouldn't b
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491512496)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It doesn't seem actually seem useful to add this `TxStateInactive::mempool_conflicted` member, so would suggest dropping it and changing definition of `isMempoolConflicted()` to just return `!mempool_conflicts.empty()`.
When I made earlier suggestion to add this member in https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1936304636 I didn't dig into the relationship between the mempool conflicte
...
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491512496)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
It doesn't seem actually seem useful to add this `TxStateInactive::mempool_conflicted` member, so would suggest dropping it and changing definition of `isMempoolConflicted()` to just return `!mempool_conflicts.empty()`.
When I made earlier suggestion to add this member in https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1936304636 I didn't dig into the relationship between the mempool conflicte
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491593170)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
This has the same problems as the new transactionAddedToMempool code above. Would suggest avoiding the need for state transitions, and always updating the mempool_conflicts set recursively for BlockConflicted as well as Inactive transactions:
```c++
if (!wtx.mempool_conflicts.erase(conflict_txid)) return TxUpdate::UNCHANGED;
return TxUpdate::CHANGED;
```
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1491593170)
In commit "wallet: track mempool conflicts" (2c867784b2a719c9e701e19e389c3adfa7d7faae)
This has the same problems as the new transactionAddedToMempool code above. Would suggest avoiding the need for state transitions, and always updating the mempool_conflicts set recursively for BlockConflicted as well as Inactive transactions:
```c++
if (!wtx.mempool_conflicts.erase(conflict_txid)) return TxUpdate::UNCHANGED;
return TxUpdate::CHANGED;
```
💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1947331502)
Ah, good point. So it looks like right now this is a refactor, and shouldn't change the binary on any platform? If you agree, you can add the `refactor: ` prefix to the pull request title.
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1947331502)
Ah, good point. So it looks like right now this is a refactor, and shouldn't change the binary on any platform? If you agree, you can add the `refactor: ` prefix to the pull request title.
💬 mzumsande commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491664859)
ok, makes sense!
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1491664859)
ok, makes sense!
👍 hernanmarino approved a pull request: "doc: Update translation process guide"
(https://github.com/bitcoin/bitcoin/pull/29414#pullrequestreview-1883922565)
ACK 3b0ec06d6228d965e9cf9121c5dd300da2a930ea
It might be interesting to add to this guide the details expressed in this comment https://github.com/bitcoin-core/gui/pull/793#issuecomment-1943421925
(https://github.com/bitcoin/bitcoin/pull/29414#pullrequestreview-1883922565)
ACK 3b0ec06d6228d965e9cf9121c5dd300da2a930ea
It might be interesting to add to this guide the details expressed in this comment https://github.com/bitcoin-core/gui/pull/793#issuecomment-1943421925
💬 DoctorBuzz1 commented on issue "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]":
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947534729)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Soo.... if we take my 2nd proposal of
`if (3Tx < TxFee) {isDust = True;}`
And adjust it just a tad to:
`if (4Tx < TxFee) {isDust = True;}`
Then that Tx would be able to be relayed & mined. Any more examples?
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947534729)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Soo.... if we take my 2nd proposal of
`if (3Tx < TxFee) {isDust = True;}`
And adjust it just a tad to:
`if (4Tx < TxFee) {isDust = True;}`
Then that Tx would be able to be relayed & mined. Any more examples?
💬 kevkevinpal commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491842216)
doesn't the `+1` make the `garbage_len` greater than `4095` which is what is being covered in `TestType.EXCESS_GARBAGE`
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491842216)
doesn't the `+1` make the `garbage_len` greater than `4095` which is what is being covered in `TestType.EXCESS_GARBAGE`
💬 kevkevinpal commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491843089)
never mind randrange doesn't include the value passed
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491843089)
never mind randrange doesn't include the value passed
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491901555)
yes!
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1491901555)
yes!
💬 DoctorBuzz1 commented on issue "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]":
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947681819)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Are you able to explain what's happening in this Tx? Why is it overpaying 3x what's necessary to get into the block when 25 sats/vByte definiely would've made it into the next block?
If the TxFee is 25,582 sats.... how is that distributed amo
...
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947681819)
> So this tx will be considered dust and won't get relayed if such solution is implemented: [bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f](https://mempool.space/tx/bdc168f1d6c38c66c762ccb36655a118436347b074f4a1ff1c8cad4e7429ae0f)?
Are you able to explain what's happening in this Tx? Why is it overpaying 3x what's necessary to get into the block when 25 sats/vByte definiely would've made it into the next block?
If the TxFee is 25,582 sats.... how is that distributed amo
...
💬 glozow commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1947800641)
Package Relay #27463
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1947800641)
Package Relay #27463
💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1947928214)
Ok, this (The signed, previous to last commit) is ready for review.
I'll drop the last commit (merge commit) once and if it is merged.
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1947928214)
Ok, this (The signed, previous to last commit) is ready for review.
I'll drop the last commit (merge commit) once and if it is merged.
💬 maflcko commented on issue "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]":
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947951310)
> a more compatible solution might be to define it as a UTXO that cannot be economically spent at the fee level of the transaction creating it (or a multiplier of said fee)
That doesn't work either, because it is trivial to side-step. For example, one could create the transaction at a lower fee rate to create the dust, then raise the fee by creating a descendant transaction with a higher fee. (The result would be that the opposite of what this proposal is trying to achieve, is achieved). Anot
...
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947951310)
> a more compatible solution might be to define it as a UTXO that cannot be economically spent at the fee level of the transaction creating it (or a multiplier of said fee)
That doesn't work either, because it is trivial to side-step. For example, one could create the transaction at a lower fee rate to create the dust, then raise the fee by creating a descendant transaction with a higher fee. (The result would be that the opposite of what this proposal is trying to achieve, is achieved). Anot
...
✅ maflcko closed an issue: "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]"
(https://github.com/bitcoin/bitcoin/issues/29423)
(https://github.com/bitcoin/bitcoin/issues/29423)
💬 maflcko commented on issue "Move from Static Dust Limit [330 / 546 sats] to Variable Dust Limit [= to TxFee]":
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947953302)
Not sure what the goal of this issue is. Anyone wanting to change the dust relay fee can already do so with the `-dustrelayfee` setting. In any case, if there is a technical sound proposal with proper motivation and a complete discussion of the effects on wallet software as well as P2P relay policy interactions, no one is holding anyone back from proposing those changes as a pull request, a BIP, or as a discussion with the broader ecosystem to gather feedback, for example https://groups.google.c
...
(https://github.com/bitcoin/bitcoin/issues/29423#issuecomment-1947953302)
Not sure what the goal of this issue is. Anyone wanting to change the dust relay fee can already do so with the `-dustrelayfee` setting. In any case, if there is a technical sound proposal with proper motivation and a complete discussion of the effects on wallet software as well as P2P relay policy interactions, no one is holding anyone back from proposing those changes as a pull request, a BIP, or as a discussion with the broader ecosystem to gather feedback, for example https://groups.google.c
...
💬 BrandonOdiwuor commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1947968598)
@theStack @hebasto @laanwj is the goal of this to generate just the missing docs or to generate all docs
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1947968598)
@theStack @hebasto @laanwj is the goal of this to generate just the missing docs or to generate all docs
💬 maflcko commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1947980558)
The goal is to add a check to the script, as described in this issue.
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1947980558)
The goal is to add a check to the script, as described in this issue.
🤔 vasild reviewed a pull request: "Improve new LogDebug/Trace/Info/Warning/Error Macros"
(https://github.com/bitcoin/bitcoin/pull/29256#pullrequestreview-1884614153)
> Make them all accept log categories, to make it possible to filter or highlight log messages from a particular component.
This seems like a step in the right direction. Currently we log messages unconditionally that don't have a category. But this is mixing two separate things - facility and severity. I have always found this confusing.
They got it right decades ago with [syslog(3)](https://www.man7.org/linux/man-pages/man3/syslog.3.html). Every log message has a facility and a severity.
...
(https://github.com/bitcoin/bitcoin/pull/29256#pullrequestreview-1884614153)
> Make them all accept log categories, to make it possible to filter or highlight log messages from a particular component.
This seems like a step in the right direction. Currently we log messages unconditionally that don't have a category. But this is mixing two separate things - facility and severity. I have always found this confusing.
They got it right decades ago with [syslog(3)](https://www.man7.org/linux/man-pages/man3/syslog.3.html). Every log message has a facility and a severity.
...