📝 pinheadmz opened a pull request: "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response"
(https://github.com/bitcoin/bitcoin/pull/32517)
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Exampl
...
(https://github.com/bitcoin/bitcoin/pull/32517)
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Exampl
...
🤔 jonatack reviewed a pull request: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2844215334)
ACK e62423d6f1514b022155edb5bc930cecc4236731
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2844215334)
ACK e62423d6f1514b022155edb5bc930cecc4236731
💬 hebasto commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091490497)
> Anything here seems fine, we don't link to https://sqlite.org/download.html or https://freetype.org/download.html.
But we do link to https://www.boost.org/users/download/ and https://github.com/libevent/libevent/releases.
I’ve ACKed the current branch and am content with it as is.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091490497)
> Anything here seems fine, we don't link to https://sqlite.org/download.html or https://freetype.org/download.html.
But we do link to https://www.boost.org/users/download/ and https://github.com/libevent/libevent/releases.
I’ve ACKed the current branch and am content with it as is.
💬 hebasto commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091491960)
It was a nit. I am OK with the current branch.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2091491960)
It was a nit. I am OK with the current branch.
⚠️ fatmirsul1234 opened an issue: "f"
(https://github.com/bitcoin/bitcoin/issues/32518)
### Motivation
f
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.
(https://github.com/bitcoin/bitcoin/issues/32518)
### Motivation
f
### Possible solution
_No response_
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...
### Guidance for new contributors
Want to work on this issue?
For guidance on contributing, please read [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.
✅ fanquake closed an issue: "f"
(https://github.com/bitcoin/bitcoin/issues/32518)
(https://github.com/bitcoin/bitcoin/issues/32518)
🚀 fanquake merged a pull request: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895)
(https://github.com/bitcoin/bitcoin/pull/31895)
💬 Sjors commented on pull request "rfc: only put replaced txs in extra pool":
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884350379)
@szarka the extra pool is capped at 100kb regardless of the number of transactions, so any filtered content over 100kb would overflow it.
It's (unfortunately) unclear from your log example what these 87 transactions were. If they were all RBF replacements, then this PR wouldn't hurt that. If they're all inscriptions, then it would.
> Seems like including things that are rejected for policy reasons is useful.
In principle yes. The problem is that when we decide a transaction is invalid f
...
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884350379)
@szarka the extra pool is capped at 100kb regardless of the number of transactions, so any filtered content over 100kb would overflow it.
It's (unfortunately) unclear from your log example what these 87 transactions were. If they were all RBF replacements, then this PR wouldn't hurt that. If they're all inscriptions, then it would.
> Seems like including things that are rejected for policy reasons is useful.
In principle yes. The problem is that when we decide a transaction is invalid f
...
💬 szarka commented on pull request "rfc: only put replaced txs in extra pool":
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884363317)
> In any case I'd love to see @0xB10C's data. I'll keep this in draft.
I think maybe [this _Delving_ post](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052) is what @glozow was referring to?
(https://github.com/bitcoin/bitcoin/pull/32510#issuecomment-2884363317)
> In any case I'd love to see @0xB10C's data. I'll keep this in draft.
I think maybe [this _Delving_ post](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052) is what @glozow was referring to?
💬 maflcko commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091545479)
nit: Not sure about making this optional out of convenience and then write the rule in the description string.
It would be better to just pass down a `std::optionl<RPCResult>` (or `std::vector<RPCResult>`) here, and then use `Cat()` to concatenate it?
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091545479)
nit: Not sure about making this optional out of convenience and then write the rule in the description string.
It would be better to just pass down a `std::optionl<RPCResult>` (or `std::vector<RPCResult>`) here, and then use `Cat()` to concatenate it?
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091547152)
I spose I'd ask the question the other way round, why not the exception? It would be an exceptional circumstance if MSBuild could not be found.
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091547152)
I spose I'd ask the question the other way round, why not the exception? It would be an exceptional circumstance if MSBuild could not be found.
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091547757)
Will try without.
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091547757)
Will try without.
💬 maflcko commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091548060)
```suggestion
if (is_change_func ) {
out.pushKV("ischange", is_change_func(txout));
```
nit: Seems more user friendly to not make the field optional and instead return a boolean?
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091548060)
```suggestion
if (is_change_func ) {
out.pushKV("ischange", is_change_func(txout));
```
nit: Seems more user friendly to not make the field optional and instead return a boolean?
💬 stickies-v commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091555297)
Oh right, they have the same bit representation, so underflowing here doesn't mean anything for serialization (and vice versa). Thanks - can be marked as resolved!
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091555297)
Oh right, they have the same bit representation, so underflowing here doesn't mean anything for serialization (and vice versa). Thanks - can be marked as resolved!
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091558808)
I mean, did GHA document handling exceptions in PowerShell scripts?
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091558808)
I mean, did GHA document handling exceptions in PowerShell scripts?
🤔 furszy reviewed a pull request: "qt, wallet: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/32238#pullrequestreview-2844335248)
Code review ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1
Happy rebase for everyone.
(https://github.com/bitcoin/bitcoin/pull/32238#pullrequestreview-2844335248)
Code review ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1
Happy rebase for everyone.
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091584150)
I started with this but then playing with the feature I made it optional because if you receive a payment you'd see a tx with a bunch of outputs that all say `ischange: false`. I felt like that was confusing because you didn't create this tx and that might imply that there is some way to detect a change output in someone else's tx.
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091584150)
I started with this but then playing with the feature I made it optional because if you receive a payment you'd see a tx with a bunch of outputs that all say `ischange: false`. I felt like that was confusing because you didn't create this tx and that might imply that there is some way to detect a change output in someone else's tx.
📝 instagibbs converted_to_draft a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage"
(https://github.com/bitcoin/bitcoin/pull/32516)
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
I didn't assert the exact order of pruning, but can if desired.
(https://github.com/bitcoin/bitcoin/pull/32516)
`DisconnectedBlockTransactions::LimitMemoryUsage()` has unit test coverage, but the default value end to end doesn't have coverage.
This test adds exercised coverage of memory limiting of the disconnect pool, and some basic behavior sanity checks.
I didn't assert the exact order of pruning, but can if desired.
💬 instagibbs commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2884446941)
I don't think I'm actually hitting that particular limit, getting weird results when looking closer, draft for now while I look
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2884446941)
I don't think I'm actually hitting that particular limit, getting weird results when looking closer, draft for now while I look
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091591988)
Not sure what you mean here -- `help gettransaction` doesn't list this at all, it just tells the user to check `decoderawtransaction`
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2091591988)
Not sure what you mean here -- `help gettransaction` doesn't list this at all, it just tells the user to check `decoderawtransaction`